Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

option to disable guessers/loaders/voters globaly #245

Merged
merged 1 commit into from
Aug 1, 2015

Conversation

ElectricMaxxx
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #242
License MIT
Doc PR symfony-cmf/symfony-cmf-docs#693

Started implementing with that and came to an easy global on/off/partial-off solution, we can start with. Doing some fance inheritance into the specific sitemap configurations will be an overkill atm - i think.

When you like that, i will do:

  • configuration schema extension
  • tests for configuration schema extension

@ElectricMaxxx ElectricMaxxx changed the title Feature/issue 242 option to disable [WIP] option to disable guessers/loaders/voters globaly Jul 28, 2015
@dbu
Copy link
Member

dbu commented Jul 29, 2015

for sitemap specific, we could have the same config in each configuration, and update the service tags with the sitemap attribute. but yeah, lets leave that out for now.

i would invert all and none, its confusing right now (the config does not say it would be about which services to skip). saying which to use, with default all sounds clearer to me.

i would make the configuration options arrayNode instead of comma separated. we can have a pre-task to convert a single string with "all" or also "...loader.phpcr" to an array.

i wonder if all/none could clash with a sitemap name. maybe use _all / _none? or something else that is very unlikely to be a sitemap? particularly all sounds a bit risky.

@dbu
Copy link
Member

dbu commented Jul 29, 2015

oh, and lets start a separate doc PR so we get the first one merged at some point...

@ElectricMaxxx
Copy link
Member Author

@dbu as the issue starts with disable ... i implemented it from the position, which sitemaps to disable :-) but i can change it if you want to.

I started implementing the configuration as an enumNode, but had problems to describe the custom list as values :-( So you really want to blow up the Configuration.php just to get the list as a pre-task? I think i can devide the values better as strings, the list is just one of the possible options, and i would have to extract all/none when they are the values.

To set the same option into each sitemap would be not the problem, to disable the services for more then one sitemap, would be very hard. I would imagine to steel the service caused by the option on one sitemap and an other still whants the service ... So lets just keep it global for now.

@ElectricMaxxx
Copy link
Member Author

@dbu i don't understand the thing with mixing sitemap names and all|none the options never touch a sitemap by its name. in the worst case a service with the id all|none can't be implemented.

@@ -1,6 +1,7 @@
Changelog
=========

* **2015-07-30**: implement options to enable/disable guessers, loaders and voters
Copy link
Member

Choose a reason for hiding this comment

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

i would just add that to the rest of the sitemap thing

@ElectricMaxxx ElectricMaxxx force-pushed the feature/ISSUE-242_option_to_disable branch from 96e959d to 6f621be Compare July 31, 2015 10:55
@ElectricMaxxx
Copy link
Member Author

@dbu we should merge that, right?

@dbu dbu force-pushed the feature/ISSUE-242_option_to_disable branch from 434fb20 to 023acde Compare August 1, 2015 09:23
@dbu dbu changed the title [WIP] option to disable guessers/loaders/voters globaly option to disable guessers/loaders/voters globaly Aug 1, 2015
@dbu
Copy link
Member

dbu commented Aug 1, 2015

i squashed commits. i think its now ready to merge. i created a separate issue for the doc, so that we don't block the initial doc PR any longer.

dbu added a commit that referenced this pull request Aug 1, 2015
…disable

option to disable guessers/loaders/voters globaly
@dbu dbu merged commit 68dcf32 into master Aug 1, 2015
@lsmith77 lsmith77 removed the wip/poc label Aug 1, 2015
@dbu dbu deleted the feature/ISSUE-242_option_to_disable branch August 1, 2015 11:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants