search

silverstripe / silverstripe-cms

Silverstripe CMS - this is a module for Silverstripe Framework rather than a standalone app. Use https://github.com/silverstripe/silverstripe-installer/ to set this up.
http://silverstripe.org/
BSD 3-Clause "New" or "Revised" License
515 stars 333 forks source link

Do not exclude routes with empty rules as URL segment candidates #2349

Open michalkleiner opened 5 years ago

michalkleiner commented 5 years ago

When trying to remove CWP search recipe imposed 'search' route, using an empty string works, however, any defined route (with or without a functioning route) gets rejected as a possible SiteTree URL segment.

CWP route definition: https://github.com/silverstripe/cwp-search/blob/master/_config/routes.yml

SiteTree URL segment exclusions: https://github.com/silverstripe/silverstripe-cms/blob/4/code/Model/SiteTree.php#L3097

Should routes with empty rules be allowed as URL segment candidates? If so, I can PR that.

michalkleiner commented 5 years ago

It seems though providing the empty rule is not working in SS4. It used to in SS3.

Is there a better way to remove a route defined by a 3rd party module?

michalkleiner commented 5 years ago

For now, using a workaround through an extension to Director.

The underlying question still stands — should Director and SiteTree skip patterns with empty rules?

tractorcow commented 5 years ago

I'm a big fan of using null as "not a value", but '' as "value, explicitly empty". Shall we filter out rules with isset?

michalkleiner commented 5 years ago

With isset check, how would you remove a yaml config (3rd party defined route) via yaml config (custom module to remove the config)? Or how do you specify null to override it via yaml?

michalkleiner commented 5 years ago

@chillu or @tractorcow can you please give me some direction so I can PR this? Thanks

chillu commented 5 years ago

So to be clear, this is about the values (usually controller names) rather than the keys (path matching rules). I think null communicates the intent better, and that's a thing in YAML as well: https://yaml.org/type/null.html. I'm not sure if our merging logic does the right thing there (takes the null value if it has higher priority), but that'd be my preference. So this wouldn't remove the config entry, it just overrides it with a null value. Which I think is easier to debug anyway.

Otherwise, I can't see how an empty string as the controller name would be used for anything but "disable this rule", so fine with that as well.