Add twig extension to update seo page #263
Conversation
benglass
commented
Sep 22, 2015
benglass
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | ??? (117 deprecation warnings but tests pass) |
| Fixed tickets | #146 |
| License | MIT |
| Doc PR | symfony-cmf/symfony-cmf-docs#705 |
- Pull requests for docs
- Determine if extension dependency on seo presentation should be moved into a SeoHelper
- Tests?
| public function getFunctions() | ||
| { | ||
| return array( | ||
| new \Twig_SimpleFunction('cmf_seo_update_seo_page', |
There was a problem hiding this comment.
why do we call this "update seo page"? i saw the php method is called that way, but the naming seems confused.
- is it not rather a set?
- and why seo page?
what about setSeoInformationFromContent($content)? and naming the twig method accordingly? we would need to keep the old php method for BC of course.
There was a problem hiding this comment.
I agree one the name, it seems to be a case where the fact that sonata seo bundle is being used is being exposed perhaps unnecessarily. I think update rather than set may be more correct, however, because some of the data is merged rather than overriden? I'm not 100% sure on this. I think you are saying this but if we are going to change the name here we should probably also change the SeoPresentationInterface method as well and deprecate the old one.
There was a problem hiding this comment.
Perhaps updateSeoMetadataFromContent($content) ?
There was a problem hiding this comment.
@dbu not sure how you are want to proceed with this. I'm happy to rename the public method in the PHP class and twig extension and add a deprecated comment, dunno if anyone else wants to weigh in on what the new name should be. I'd lean towards Metadata vs. information for consistency but I can see the case for set or update.
There was a problem hiding this comment.
sorry for not being responsive, i was away. i created #264 as its an interface change.
for the twig function, could we call it cmf_seo_update_from_document? the name you currently have looks weird with the repeated seo. and i rather have a good name here than exact naming consistency with something we agree is not well named.
@ElectricMaxxx wdyt?
There was a problem hiding this comment.
ups, you are correct. cmf_seo_update_from_model then? (i guess i know why we ended up with "page" before, but i think page is weird. its probably because in sonata they where thinking about pages from SonataPageBundle)
There was a problem hiding this comment.
so this would be cmf_seo_update_from. a bit weird. maybe cmf_seo_update_metadata?
There was a problem hiding this comment.
Should we just use the same name for PresentationInterface and this, your recommended updateMetadata
SeoPresentation::updateMetadata($content)
cmf_seo_update_metadata($content)
There was a problem hiding this comment.
yes, thats it. lets go with that. can you update this PR? the work on 2.0 has not yet started, so #264 does not have to happen soon.
There was a problem hiding this comment.
Renamed to cmf_seo_update_metadata and docs PR updated
|
this looks good, thanks! can you please add a note to the CHANGELOG.md file and squash the commits? if you want to add a test, go for it but i can merge without it. |
|
the deprecation warnings are because of dependencies like sonata. |
Add tagged service definition for twig extension Add note to changelog
benglass
force-pushed
the
add_twig_extension_update_seo_page
branch
from
ffe6d46 to
c1c3467
Compare
|
@dbu Commits squash, changelog updated. I don't think I'm likely to get tests together for this any time soon and it doesnt really seem like its doing very much (just proxying to another method). |
Add twig extension to update seo page
|
thanks ben! |
