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 the parameter conversion for non-loaded metadata #6210

Open
wants to merge 1 commit into
base: 2.6
Choose a base branch
from

Conversation

stof
Copy link
Member

@stof stof commented Jan 3, 2017

Closes #6209

@stof
Copy link
Member Author

stof commented Jan 3, 2017

2 things here:

  • I have kept the call to hasMetadataFor for performance reasons for the common use case (as isTransient always initializes the driver-level API in the factory). A PR may be done to doctrine/common to optimize isTransient based on loaded metadata.
  • I have not written tests for this case, because I don't know how to write it easily: it requires doing a cache hit in the DQL parser cache before loading metadata in the factory (or passing an object not matching the type of the mapped property so that the DQL parser loads a different metadata than the param, but this would mean writing non-sense code), which probably implies using 2 different EntityManager instances sharing the same DQL cache in the test (but not the same metadata factory so that they load metadata separately) or resetting the internal cache of the factory at some point.

@Ocramius
Copy link
Member

@stof a decent way to test this could be to use generated entity class names, which change at each test execution, with:

  1. a class that is mapped
  2. a class that is not mapped

This would need to go through eval().

@stof
Copy link
Member Author

stof commented Jun 15, 2018

@Ocramius that would not help, as we would not have a hit on the DQL parser cache then (as the class name is part of the DQL).

@stof
Copy link
Member Author

stof commented Jun 15, 2018

The root cause of the issue is that hasMetadataFor does not check whether this class is mapped (this is what isTransient does) but whether the factory has loaded metadata for the class (so if it says true, we are sure that it is a mapped class, but if it says false, we cannot know whether it is mapped or unmapped).

@Ocramius
Copy link
Member

@stof my suggestion to use generated class names is exactly because you want to have both scenarios of hasMetadataFor (loaded and not loaded)

@stof
Copy link
Member Author

stof commented Jun 15, 2018

@Ocramius but if I have a cache miss on the DQL parser, I won't have the issue, because the parsing will load metadata before the parameter processing happens (and so metadata will be loaded at this point if the mapped class is involved in any place related to the query)

@Ocramius
Copy link
Member

@stof that's why two generated classes are needed: one to be loaded by the driver, one to be ignored.

@stof
Copy link
Member Author

stof commented Jun 15, 2018

@Ocramius But for it to represent an actual case (and not a hacky meaningless usage just to trigger the bug), I would have to write a query which involves the class of the object (you would use this object parameter in a place checking a relation to its class, not a relation to another one). The only way to create such case then would be to have the DQL being already parsed in the cache (parsing DQL loads metadata to validate the fields being used and to convert the DQL to SQL).

Btw, there is no need to use eval'd class. The issue is not for the class not being loaded by the autoloader (if you pass an object here, its class is always loaded), but about metadata being loaded in memory already.

@Ocramius
Copy link
Member

The issue is not for the class not being loaded by the autoloader (if you pass an object here, its class is always loaded), but about metadata being loaded in memory already.

The generated class is just forcing the system to create a new symbol that isn't yet known to the metadata factory, nothing to do with autoloading.

I misunderstood that this is related to caching - not sure how to create a cache for a (later on) unknown metadata symbol.

@stof stof changed the base branch from master to 2.6 September 24, 2018 16:24
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.

None yet

2 participants