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

Cleanup invalidation annotation #121

Merged
merged 3 commits into from Aug 2, 2014
Merged

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Aug 1, 2014

To prepare for #117 i wrote some more functional tests. One big wtf though.

Also split the tag specific controller into a separate controller for better readability.

And make InvalidateRoute explicit about expression.

@@ -6,3 +6,4 @@
}

$autoload = require_once $file;
Doctrine\Common\Annotations\AnnotationRegistry::registerLoader(array($autoload, 'loadClass'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is according to the last line in the installation section of the symfony doc for the sensio bundle.

seems to make sense - but my huge wtf is why the functional test for @Tag works without this line. the @InvalidationRoute|Path do not work without the line.

Copy link
Member

Choose a reason for hiding this comment

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

Really weird indeed! Glad you got the annotation tests working, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i add a note to the annotations reference

@ddeboer ddeboer mentioned this pull request Aug 1, 2014
6 tasks
print_r($value, true)
));
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doing the validation early to notice problems as soon as possible

@ddeboer
Copy link
Member

ddeboer commented Aug 1, 2014

Thanks! I don’t particularly like the way expression route params look now as they take up much space. Consistency, with @Tag(expression=...), should probably win here, though, so please merge if you consider this done.

* @InvalidatePath("/posts")
* @InvalidatePath("/posts/latest")
* @InvalidatePath("/articles")
* @InvalidatePath("/articles/latest")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed all examples to use /articles rather than /posts to avoid any confusion with the http POST method (i was confused while jumping around in the doc for a sec, so thought for somebody new to all this, its better to use this name)

Copy link
Member

Choose a reason for hiding this comment

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

Good one.

@dbu
Copy link
Contributor Author

dbu commented Aug 2, 2014

Okay, i think thats it. Please check the additional changes i did. Then lets merge and tag beta of component and bundle, ok?

ddeboer added a commit that referenced this pull request Aug 2, 2014
…notation

Cleanup invalidation annotation
@ddeboer ddeboer merged commit 11ea3ea into master Aug 2, 2014
@ddeboer ddeboer deleted the cleanup-invalidation-annotation branch August 2, 2014 10:07
@ddeboer
Copy link
Member

ddeboer commented Aug 2, 2014

Looks good, thanks!

@@ -181,6 +181,10 @@ private function addMatch(NodeBuilder $rules)
->ifTrue(function ($v) {return !empty($v['additional_cacheable_status']) && !empty($v['match_response']);})
->thenInvalid('You may not set both additional_cacheable_status and match_response.')
->end()
->validate()
->ifTrue(function ($v) {return !empty($v['match_response']) && !class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage');})
->thenInvalid('Configured a match_response but ExpressionLanugage is not available')
Copy link
Member

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, fixed in 8597598

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