search

silverstripe / silverstripe-redirectedurls

Silverstripe module to let users to configure arbitrary redirections in the CMS
BSD 3-Clause "New" or "Revised" License
32 stars 48 forks source link

Remove forced lower case on all fields #50

Closed indygriffiths closed 3 years ago

indygriffiths commented 7 years ago

This pull request:

dhensby commented 7 years ago

Please rebase on latest master too so that the 5.3 tests run :)

indygriffiths commented 7 years ago

First time contributing to open source repo, so apologies for the mess of commits!

I've added a config option to allow case insensitive query parameters, and created a new test that checks if the redirect happens when case insensitive matching is enabled and the query parameters use a different case to the redirect rule.

dhensby commented 6 years ago

Thanks a lot for the hard work on this and sorry I've been silent the past couple of weeks.

Just a few more changes :)

dhensby commented 6 years ago

@robbieaverill would you mind taking a look at this?

dhensby commented 6 years ago

@indygriffiths the master branch is now SS4 compat. I've set this PR to target 1 branch now.

I haven't looked at this for a while so I'd like to clarify a few things.

  1. The initial issue was the we were lowercasing the To URL on save which meant redirects would not go to the URL as entered by the CMS admin (if they had uppercase chars).
  2. This fix now stores URLs as entered by the CMS user
  3. This fix now optionally (by default?) matches the From URL case insensitively
  4. The fix retains the case for the To URL at all times

Is that all correct?

indygriffiths commented 6 years ago

@dhensby

  1. Yes, the original issue stems from a client where URLs are mixed case and aren't handled correctly as they are all being saved as lower case.
  2. Yes, this PR removes the forced lower case when saving the base URL and query string.
  3. The config option is for the case sensitivity of the URL parameters, not the entire URL. By default this is set to case sensitive matching, as per W3 Before this change they were also being forced to lower case. The FromBase is also case sensitive.
  4. Yes, the case for the To was never being changed.
dhensby commented 6 years ago

OK - so we are only match the query string in a case insensitive way? That seems unnecessary...

I'd prefer if we had it mimic current behaviour (whole link is checked in a case insensitive way) and that this be default (as it is currently) and devs can opt-out of loose matching.

Otherwise we run the risk of unexpected regressions