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

implemented missing methods, fixes issue #28 #29

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

youbs
Copy link

@youbs youbs commented Jun 21, 2012

These are @jonathaningram changes (plus the finder as suggested by @schmittjoh)

@fabpot
Copy link
Contributor

fabpot commented Jun 21, 2012

@schmittjoh Can you have a look as soon as possible as I'm waiting for this fix to release 2.1.0 beta1. Thanks.

@schmittjoh
Copy link
Owner

I can't verify this patch until I'm home, but apparently it does not work judging from the comments on #28.

'finder' => $this->finder,
'filePattern' => $this->filePattern,
'files' => $this->files,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to save $this->directories too

Copy link
Author

Choose a reason for hiding this comment

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

thx

public function serialize()
{
$resourceMap = array(
'finder' => $this->finder,
Copy link
Contributor

Choose a reason for hiding this comment

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

do you really need to serialize the finder ? It is not necessary to rebuild the resource as it is instantiated inside the class anyway. So it could be recreated in unserialize(), making the serialized data smaller

Copy link
Author

Choose a reason for hiding this comment

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

indeed, I'll update the PR, thank you

@fabpot
Copy link
Contributor

fabpot commented Jun 21, 2012

I've reverted the code from Symfony master for now as it introduces too many regression. Of course, this PR is still important as the code will be reintroduced for Symfony 2.1 beta1.

@schmittjoh
Copy link
Owner

@fabpot, do you think the interface will change until then?

@stof
Copy link
Contributor

stof commented Jun 22, 2012

until beta1 ? no as it is released :)
I will be for beta2 in fact

@nicolas-bastien
Copy link

Hello so now when I update my project via composer I get the error :

Warning: Class JMS\DiExtraBundle\Config\FastDirectoriesResource has no unserializer

Do you miss something to merge this PR ?

@jonathaningram
Copy link

@nicolas-bastien just double check you rm -rf app/cache/dev - i.e. manually remove the cache just in case.

@nicolas-bastien
Copy link

@jonathaningram thanks I did it but as I have copied youbs change before I thought the correction came from this and not my cache !
So it's ok I don't have the problem anymore, thanks.

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.

6 participants