Skip to content

Drop CachedReader#249

Merged
Ocramius merged 1 commit intodoctrine:masterfrom
Majkl578:drop-CachedReader
Feb 26, 2019
Merged

Drop CachedReader#249
Ocramius merged 1 commit intodoctrine:masterfrom
Majkl578:drop-CachedReader

Conversation

@Majkl578
Copy link
Copy Markdown
Contributor

See #204 (comment) for reasoning.

Closes #204.

@Majkl578 Majkl578 added this to the v2.0.0 milestone Feb 25, 2019
Comment thread tests/Doctrine/Tests/Annotations/PerformanceTest.php Outdated
@alcaeus
Copy link
Copy Markdown
Member

alcaeus commented Feb 26, 2019

FWIW, I agree with dropping CachedReader in its current form, but think we should still provide a CachedReader.

For one, caching is not "marginally" useful. While it makes sense that resulting structures (e.g. ORM/ODM metadata) should be cached, this assumes that only a single consumer will ever read annotations for a class when in fact multiple packages may want to read their annotations from the same class.

To bring a concrete examples, here are the different types of annotations found in a single class:

  • ODM metadata
  • JMS serialiser metadata
  • Algolia search indexing definitions
  • Validation constraints

I believe providing a small class that allows caching read annotations for classes in a PSR-6 or PSR-16 cache is something we should definitely do. Providing such a class also doesn't cause a huge dependency tree overhead as we'd only have to require the PSR interfaces without pulling in or even suggesting a concrete cache implementation. Providing a CachedReader implementation also avoids multiple frameworks having to re-implement this which they'd most likely do if we didn't ship such a reader.

I'll leave it up to @Majkl578 to decide whether to merge this and re-introduce a new CachedReader or whether to close this PR in favour of changing the existing implementation.

lcobucci
lcobucci previously approved these changes Feb 26, 2019
Copy link
Copy Markdown
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is a great thing. Most libraries that rely on annotations have their own cache structure.
Having a caching layer for "temporary information" (as in: only used to create configuration objects) doesn't add much IMHO.

@Majkl578
Copy link
Copy Markdown
Contributor Author

Majkl578 commented Feb 26, 2019

@alcaeus While I had the same opinion as you, it's true that annotations are a format to describe stuff, pretty much like XML or JSON or YAML. So in the end the annotations reader is an intermediary tool which should only provide transient data that are transformed into library/application-specific metadata.
Also from what I've seen often the CachedReader is used together with ArrayCache which hardly brings any benefit anyway.

So IMO removing it is a good call, and in case anyone provides a valid use case we can add new caching decorator on top of PSR-6/16, ideally as a separate package (annotations-cache).

@alcaeus
Copy link
Copy Markdown
Member

alcaeus commented Feb 26, 2019

@Majkl578 very well then 👍

alcaeus
alcaeus previously approved these changes Feb 26, 2019
Copy link
Copy Markdown
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Question on when to turn master into 2.0.0 remains (should I release 1.7.0 and call it a day for 1.x?)

@Ocramius
Copy link
Copy Markdown
Member

As per discussion with @alcaeus and @Majkl578, master is already 2.0.0, so this is going there.

@Ocramius Ocramius self-assigned this Feb 26, 2019
Copy link
Copy Markdown
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Majkl578 this requires a rebase, since the performance tests still have the CachedReader in there

@Majkl578 Majkl578 dismissed stale reviews from alcaeus and lcobucci via 05d9476 February 26, 2019 15:45
@Majkl578
Copy link
Copy Markdown
Contributor Author

Rebase done.

@Ocramius Ocramius merged commit b25b38f into doctrine:master Feb 26, 2019
@Ocramius Ocramius changed the title Drop CachedReader Drop CachedReader Feb 26, 2019
@Majkl578 Majkl578 deleted the drop-CachedReader branch February 26, 2019 15:53
@Majkl578 Majkl578 mentioned this pull request Apr 3, 2019
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants