-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
31f03eb
to
b3f9ba3
Compare
->arrayNode('templates') | ||
->addDefaultsIfNotSet() | ||
->children() | ||
->scalarNode('xml')->defaultValue('CmfSeoBundle:Sitemap:index.xml.twig')->end() |
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.
i think this should be an array with the format as key, so that users can add even keys that the bundle knows nothing about. the controller is built that way. the format should look like this: https://github.com/symfony-cmf/RoutingBundle/blob/master/DependencyInjection/Configuration.php#L41-L45 but with useAttributeAsKey('format')
not sure if we can put the defaults in the Configuration class in this case - maybe something like ->beforeNormalization()->ifEmpty()->then(function ($v) { return array('xml' => ...); })->end()
(did not test this so might not work exactly like this)
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.
+1 for a prototyped array
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.
I agree too, I'll test if default value in this case can work
although overwriting templates would be fine in this case, i think this idea is right because it allows to add aditional formats (e.g. a template for json, or for a custom sitemap format) |
What is wrong with overwriting templates by the simple "symfony way"? |
@ElectricMaxxx ability to add custom formats (when using a prototyped array) |
You can add what you whant when injecting it into the service defintion. But you yould probably right, using that parameter array, injecting that is easier to use. |
@ElectricMaxxx as is, one would have to write a compiler pass to change the service definition. a configuration option is more convenient |
👍 (Btw: i think i have had it that way somewhere in the Monster PR g) |
You know how to work on that PR or need some help? |
This one works, but I don't know how (for example) to keep 'xml' default value if the html one is overrided. |
@@ -90,6 +90,15 @@ public function getConfigTreeBuilder() | |||
->canBeEnabled() | |||
->children() | |||
->scalarNode('default_chan_frequency')->defaultValue('always')->end() | |||
->arrayNode('templates') |
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.
you should add ->fixXmlConfig('template')
before line 91
I would like to skip that one here, cause i will re-implement it when introducing named sitemaps and its values (templates, enhancers, ...) in #221 |
i ported this into master, now that the refactoring happened. @Peekmo please have a look at master and open a new issue if you think there is something still missing. thanks for the contribution! |
Hello,
To be able to override or add some templates, I added the key "templates" in config (much cleaner I suppose).
Let me know if something is wrong with it :)
Regards,
Peekmo