-
Notifications
You must be signed in to change notification settings - Fork 27
[WIP] [2.0] Introduced the concept of loaders #294
base: master
Are you sure you want to change the base?
Conversation
Will have a closer Look into the code later, for now i miss the PR template. Thought it's added by github for every commiter. |
@ElectricMaxxx it's because there is a typo in the template file name |
Updated the PR with an annotation loader |
@@ -42,7 +42,7 @@ | |||
<argument type="service" id="sonata.seo.page" /> | |||
<argument type="service" id="translator" /> | |||
<argument type="service" id="cmf_seo.config_values" /> | |||
<argument type="service" id="cmf_seo.cache" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the caching now? Simply done by the anotation metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah it is one of the todo's , sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extractors are no longer managed by SeoPresentation, but in a specialized ExtractorLoader
. Caching was caching which extractors should be executed for which classes, so caching now is the task of this loader.
As the SEO values can be changed by an admin, it doesn't make sense to cache them.
cool. Do read it right, that the former extraction system will stay and you just put it into its own loader? |
@ElectricMaxxx yes, that's correct. This way, we can deprecate it during the 2.x lifecycle and get rid of it completely in 3.x |
d9f6cfb
to
3e8c4b1
Compare
Finished and tested the cache integration. This should be ready for a final review now. |
@@ -0,0 +1,7 @@ | |||
Symfony\Cmf\Bundle\SeoBundle\Tests\Resources\Document\ContentWithExtractors: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be removed (as well as empty.yml)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about that?
This is the start of supporting annotations and mapping files instead of PHP extractor methods and lots of interfaces.
friendly ping @ElectricMaxxx, do you have some time to review this? |
@ElectricMaxxx no problem, let's just simply wait till you're back :) This PR isn't holding back any other PRs/progress |
very friendly ping @ElectricMaxxx |
global: | ||
- SYMFONY_DEPRECATIONS_HELPER=weak | ||
|
||
matrix: | ||
include: | ||
- php: 5.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is LTS for 5.6 over now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, 5.6 is still tested (see the PHP version list). However, when testing all different supported Symfony versions, we should use the latest version around (as it's the quickest).
@wouterj found some smal issues, but then it looks good. |
applied the comments |
This is the start of supporting annotations and mapping files instead of
PHP extractor methods and lots of interfaces.
Most of this PR is just about moving methods and code around.
Todo
/fixes #133