Skip to content

Fix DefaultRouteGenerator cache tests with Symfony 3.1#3899

Merged
soullivaneuh merged 1 commit intosonata-project:3.xfrom
soullivaneuh:sf-3.1-fix
May 31, 2016
Merged

Fix DefaultRouteGenerator cache tests with Symfony 3.1#3899
soullivaneuh merged 1 commit intosonata-project:3.xfrom
soullivaneuh:sf-3.1-fix

Conversation

@soullivaneuh
Copy link
Member

@soullivaneuh soullivaneuh commented May 31, 2016

Subject

Symfony\Component\Config\Resource\FileResource fail if the given path is invalid since Symfony 3.1.

It was the case from the begining because of mocking an interface.

A condition is added to the RoutesCache class.

Ref: phpDocumentor/ReflectionDocBlock#72 (comment)

@OskarStark
Copy link
Member

LGTM 👍

@soullivaneuh
Copy link
Member Author

Seems to not be enough...

@soullivaneuh
Copy link
Member Author

I have to change my logic by updating the RoutesCache to get tests working.

This should not influence anything on the codebase since a bad filepath should not be loaded at all.

@sonata-project/contributors I need a review for this one.

Symfony\Component\Config\Resource\FileResource fail if the given path is invalid since Symfony 3.1.

It was the case from the begining because of mocking an interface.

A condition is added to the RoutesCache class.

Ref: phpDocumentor/ReflectionDocBlock#72 (comment)
@greg0ire
Copy link
Contributor

greg0ire commented May 31, 2016

So if I understand well, the tests are failing because we mock the Admin, but do not mock FileResource or the reflection api (how could we? they are not injected in the code…). I think this is a good temporary solution if it make the build green again, but maybe we should think about not coupling the RoutesCache to the filesystem, (and / or improve tests by mocking the filesystem ?)

@stof
Copy link
Contributor

stof commented May 31, 2016

@greg0ire the tests are failing because of a real bug in the code. The usage of a mock only triggers this bug (because mock classes are defined through eval rather than being defined in a file)

@soullivaneuh
Copy link
Member Author

@greg0ire the tests are failing because of a real bug in the code.

@stof Could you please elaborate? The code was failing because the filepath given by reflection of the Mocked Admin class returns phar:///home/travis/bin/phpunit/phpunit-mock-objects/Framework/MockObject/Generator.php(310) : eval()'d code

Example: https://travis-ci.org/sonata-project/SonataAdminBundle/jobs/134243354#L500

I merge this for the moment. But I'll open an issue based on your answer. 👍

@soullivaneuh soullivaneuh merged commit 43b1359 into sonata-project:3.x May 31, 2016
@soullivaneuh soullivaneuh deleted the sf-3.1-fix branch May 31, 2016 18:53
@greg0ire
Copy link
Contributor

greg0ire commented May 31, 2016

@greg0ire the tests are failing because of a real bug in the code.

Thanks for the explanation @stof , if I get it right a proper fix would be to not use FileResource in this case but another implementation of ResourceInterface, that would return that "eval()'d" code when calling its __toString method. I don't see any here, so I guess we would have to create our own, is that right ?

@stof
Copy link
Contributor

stof commented Jun 1, 2016

@greg0ire the relevant things is not what it does in its __toString method (though return eval()'d only will cause issue with the deduplication of resources) but what it does to check for freshness.
checking for freshness of eval'd code is quite hard (and so skipping adding a resource is acceptable, as this will generally happen only in tests anyway)

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

Glad to learn we're fine 😅 I indeed think making this work for tests only is not worth it.

greg0ire pushed a commit to greg0ire/SonataAdminBundle that referenced this pull request Jun 2, 2016
…t#3899)

Symfony\Component\Config\Resource\FileResource fail if the given path is invalid since Symfony 3.1.

It was the case from the begining because of mocking an interface.

A condition is added to the RoutesCache class.

Ref: phpDocumentor/ReflectionDocBlock#72 (comment)
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