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

Implementing __set_state for CakeRoute #4953

Merged
merged 3 commits into from Oct 28, 2014

Conversation

Projects
None yet
5 participants
@jrbasso
Copy link
Member

jrbasso commented Oct 22, 2014

It helps the applications to cache the routes using var_export when possible.

Implementing __set_state for CakeRoute
It helps the applications to cache the routes using var_export when possible.

@jrbasso jrbasso added the routing label Oct 22, 2014

@jrbasso jrbasso added this to the 2.6.0 milestone Oct 22, 2014

@josegonzalez

This comment has been minimized.

Copy link
Member

josegonzalez commented Oct 22, 2014

PHPCS failures, and a possible bs failure in PG.

@jrbasso jrbasso force-pushed the jrbasso:2.6-route-set-state branch from cd70ba4 to f7320cd Oct 22, 2014

@jrbasso

This comment has been minimized.

Copy link
Member Author

jrbasso commented Oct 22, 2014

PHPCS fixed. The issue with PG is happening for all builds on 2.6 derived branch.

@josegonzalez

This comment has been minimized.

Copy link
Member

josegonzalez commented Oct 22, 2014

I blame @lorenzo :P

@lorenzo

This comment has been minimized.

Copy link
Member

lorenzo commented Oct 22, 2014

Why use var_export instead of serialize() ?

@jrbasso

This comment has been minimized.

Copy link
Member Author

jrbasso commented Oct 22, 2014

@lorenzo That's a good question. My brain was half-off and I copied the solution from FastRoute (https://github.com/nikic/FastRoute/blob/master/src/functions.php#L54-L58)

@lorenzo

This comment has been minimized.

Copy link
Member

lorenzo commented Oct 23, 2014

Ah well, I see one reason to do it like that. If you do it like fast route, the opcache will optimise that file, whereas doing it with serialize() will not allow that.

@jrbasso

This comment has been minimized.

Copy link
Member Author

jrbasso commented Oct 23, 2014

That's true. I haven't thought about the opcache, but makes sense. Also, probably if you have lots of routes it will generate a gigantic string and it will take a while for the php to parse it before build the objects.
Probably serialize would be a best solution if you have just few routes (which probably you don't need cache), or in cases you don't have access to write on the disk, or in cases where you want to share the cache between multiple machines.

*
* @return void
*/
public function testSetState() {

This comment has been minimized.

@AD7six

AD7six Oct 27, 2014

Member

Maybe I'm caffeine deficient, but this test isn't testing setState. Whether the function CakeRoute::__set_state is defined or not this test will pass, it's not really testing anything at all.

The result of calling CakeRoute::__set_state is what should be being tested.

This comment has been minimized.

@jrbasso

jrbasso Oct 27, 2014

Author Member

Are you caffeinated now? :)

The test uses var_export() which will dump the result like CakeRoute::__set_state(['key' => 'value']). So doing an eval() of it will trigger the code I implemented and generate a new object. PHPUnit assertEquals compare the object generate manually with the object generated from the CakeRoute::__set_state(), attribute by attribute.

That makes sense? I can make a simpler test just generating an object from calling the __set_state directly, but I thought this way would cover more the real usage of __set_state.

This comment has been minimized.

@AD7six

AD7six Oct 27, 2014

Member

Ah I see =).

I'd still prefer to see a more simple, explicit test, but otherwise a comment to explain what the test is doing would be a great help.

This comment has been minimized.

@markstory

markstory Oct 28, 2014

Member

Having a test that doesn't use eval() would be nice 😄

This comment has been minimized.

@jrbasso

jrbasso Oct 28, 2014

Author Member

Tests updated.

Update test cases for CakeRoute::__set_state
It removes the eval and test the __set_state more explicitly.

@jrbasso jrbasso force-pushed the jrbasso:2.6-route-set-state branch from d9dbc79 to 6171ef6 Oct 28, 2014

@markstory

This comment has been minimized.

Copy link
Member

markstory commented Oct 28, 2014

Looks good to me 👍

@AD7six

This comment has been minimized.

Copy link
Member

AD7six commented Oct 28, 2014

👍

lorenzo added a commit that referenced this pull request Oct 28, 2014

Merge pull request #4953 from jrbasso/2.6-route-set-state
Implementing __set_state for CakeRoute

@lorenzo lorenzo merged commit 9d0390c into cakephp:2.6 Oct 28, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@lorenzo lorenzo deleted the jrbasso:2.6-route-set-state branch Oct 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.