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

Properly wakeup ClassMetadata #129

Merged
merged 3 commits into from
Feb 22, 2014
Merged

Properly wakeup ClassMetadata #129

merged 3 commits into from
Feb 22, 2014

Conversation

bakura10
Copy link
Member

ping @Ocramius

Resource metadata holds reference to a classmetadata. However, ClassMetadata reflection fields (reflFields) are not serialized, which means that if we use APC to cache ZfrRest resource metadata, the ClassMetadata from Doctrine is not properly wake up and it crashes.

This PR fixes that by regenerating missing fields. I know it introduces a dependency to Doctrine ORM but this seems like the lightest way.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5438beb on fix-cache into 984a0bf on master.

@Ocramius
Copy link
Member

Le awesome commit message :3

$classMetadata = $this->propertyMetadata['classMetadata'];

// @TODO: it introduces a coupling with ORM... but that seems the only way
if ($classMetadata instanceof ClassMetadataInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

The ORM metadata factory handles this - maybe avoid wrapping the metadata and fetch it from there via a name reference?

@bakura10
Copy link
Member Author

Better @Ocramius ? Didn't tested it yet, but it removes dependency to ORM.

@bakura10
Copy link
Member Author

I updated tests @Ocramius . Can you do a quick review?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8f36976 on fix-cache into 984a0bf on master.

@bakura10
Copy link
Member Author

I'm merging, I need this :). If you have any feedback, I'll fix them then.

bakura10 added a commit that referenced this pull request Feb 22, 2014
Properly wakeup ClassMetadata
@bakura10 bakura10 merged commit 4cb0af6 into master Feb 22, 2014
@bakura10 bakura10 deleted the fix-cache branch February 22, 2014 14:30
*/
public function serialize()
{
unset($this->propertyMetadata['classMetadata']);
Copy link
Member

Choose a reason for hiding this comment

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

You are still putting the metadata in here as it seems

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh? I removed it when I serialize the resource metadata because I recreate it in the factory.

Copy link
Member

Choose a reason for hiding this comment

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

It will break if you unserialize the metadata, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhhh no I don't think. However this PR break everything. I'm investigating.

@Ocramius
Copy link
Member

Looking good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants