Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Metadata Caching when it changes in EventListeners #42

Open
wants to merge 6 commits into
base: 1.1.x
Choose a base branch
from

Conversation

andrey-bondar-dron
Copy link

I've closed PR #21 and created new one, because that PR is targeted on master.

This PR fixed 2 bugs:

  1. Using https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/cookbook/resolve-target-entity-listener.html with metadata cache is not working, because ResolveTargetEntityListener calls setMetadataFor without any effect on generated cache.
  2. Fallback metadata is not caching now.

@dron-alterpost dron-alterpost force-pushed the fix-metadata-caching-1-1-x branch from 2e1a160 to af490ed Compare February 18, 2019 21:53
Pavlentiycomua
Pavlentiycomua previously approved these changes Feb 18, 2019
Copy link
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.

Solution looks valid, but we need to duplicate entries in aliasesMap to simplify "happy path" lookups, and reduce overhead there.


if ($pos === false) {
return $class;
switch (true) {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use switch(true). Two if blocks are a better solution here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

@Ocramius Ocramius added the Bug Something isn't working label Feb 21, 2019
*/
private function getRealClass(string $class) : string
protected function getRealClassName(string $className) : string
Copy link
Member

Choose a reason for hiding this comment

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

This should most likely stay private

Copy link
Member

Choose a reason for hiding this comment

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

This is protected as per suggestion of @alcaeus at #21 (comment)

{
$pos = strrpos($class, '\\' . Proxy::MARKER . '\\');
if (isset($this->aliasesMap[$className])) {
Copy link
Member

Choose a reason for hiding this comment

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

Considering that duplicate entries are allowed, is this property still needed?

Copy link
Member

Choose a reason for hiding this comment

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

This was done because of #21 (comment) Can you please decipher comment chain and explain if something still needs to be done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants