Allow disabling of content listener. #209
Conversation
|
looks sane to me, and makes sense. the only thing missing imo is a PR to adjust the doc, with a |
| @@ -85,6 +85,9 @@ public function getConfigTreeBuilder() | |||
| ->scalarNode('enable_sibling_provider')->defaultValue(false)->end() | |||
| ->end() | |||
| ->end() | |||
| ->arrayNode('content_listener') | |||
| ->canBeDisabled() | |||
There was a problem hiding this comment.
Instead, I think we should use a booleanNode and call it use_content_listener
There was a problem hiding this comment.
also possible. but if we ever want to have other configuration on content_listener, it will be unelegant. then again that is not too likely, so +1 for use_content_listener (without a content_listener array node)
There was a problem hiding this comment.
This is actually how I originally wrote it (booleanNode) however my thought was we may want to allow specifying a service_id to use instead of the out of the box content listener so you could provide your own content listener and not have to rewrite the alternate locale provider portion. I'm perfectly happy to switch it to a booleanNode, just figured I'd provide my rational first
There was a problem hiding this comment.
While we agree, I just wanted to point out that you can easily keep BC by keeping content_listener.enabled and use_content_listener in sync in case we want to add more config in the future.
There was a problem hiding this comment.
Hmm, what about a content_listener setting (scalarNode) which could either be the service id or false/null to disable it?
There was a problem hiding this comment.
Default value would be thre service id of thre drfault listener.
There was a problem hiding this comment.
I like this its more explicit
There was a problem hiding this comment.
does the bundle need to know the service id of the default listener? if yes that would be the right thing to do. although, would that mean that if its the default value, SeoBundle loads the service and otherwise its assumes the application provides the service somewhere else? and, does the SeoBundle need to know the name of that listener, does it inject it elsewhere?
There was a problem hiding this comment.
@dbu I think the way this is handled by FOSUserBundle where you can override the service id for the user manager is that the default service's id is like cmf_seo.content_listener_default and the dependency injection extension sets up cmf_seo.content_listener as an alias based on the configuration
|
The XML schema needs to be updated too |
|
I think there a still some features missing in the xml schema :-( |
|
@benglass looks good to me,but can you add a test for both situations. Should be something like |
|
@benglass any chance to wrap this up? we think about tagging a new minor version of SeoBundle this week to get the sitemap generation feature in a stable release. |
| 'setAlternateLocaleProvider', | ||
| array($this->container->getDefinition('cmf_seo.alternate_locale.provider_phpcr')) | ||
| ); | ||
| if ($this->container->has('cmf_seo.event_listener.seo_content')) { |
There was a problem hiding this comment.
Because this PR adds the ability to remove the seo content listener, therefore this test needs to only assert that the event listener exists with this method call if the event listener exists
There was a problem hiding this comment.
normally the alternate locale provider is injected into the SeoPresentation, when activated and with phpcr as implementation, this is done inside of the CmfSeoExtension. I would propose to add your changes there.
There was a problem hiding this comment.
Do then you should use the config section above of the test and check for the existence by on or many asserts. Cause i think your/my event listener will exist only, when there is a line in the configuration, right?
|
I have now added tests and updated the xml schema. I do not have experience working with xsd files so can someone verify I did it correctly? The current schema file does not seem up to date with the other config values but I think you may have already fixed this in the master branch? Doc PR will be coming but it would be great to get this merged in for a numbered release |
| @@ -39,4 +39,8 @@ | |||
| <xsd:enumeration value="canonical" /> | |||
| </xsd:restriction> | |||
| </xsd:simpleType> | |||
|
|
|||
| <xsd:simpleType name="enable_content_listener"> | |||
There was a problem hiding this comment.
these changes should be removed and instead, you should add this between line 20 and 21 in this file:
<xsd:attribute name="enable-content-listener" type="xsd:boolean" default="true" />
|
Thanks @benglass! Apart from my XSD comment, it looks great. Could you please also rebase this PR? Then Travis can run a build and we can merge if Travis is happy :) |
benglass
force-pushed
the
allow_disabling_listener
branch
2 times, most recently
from
b494402 to
fb0f53a
Compare
benglass
force-pushed
the
allow_disabling_listener
branch
from
fb0f53a to
6458665
Compare
|
@wouterj Squashed, rebase and awaiting tests |
|
@wouterj @ElectricMaxxx @dantleech Tests passing ready for merge |
|
👍 |
Allow disabling of content listener.
|
thanks a lot! created an issue so we don't forget the documentation: symfony-cmf/symfony-cmf-docs#644 |
We are working on integrating this into our CMS but would like to use our own content listener.
TO DO