SS4 Compatibility

[dhensby]

todo:

• [x] Get travis working
• [x] Set up codecov
• [x] Set up scrutinizer (if not done)
• [x] Add code style checking
• [x] Update docs

[dhensby]

Right, so we have a failure with the code style checker for some reason - it runs fine locally and this was copied from other SS repos so I'm not clear on why this is failing.

It appears codecov is set up (https://codecov.io/gh/silverstripe/silverstripe-redirectedurls/) but the hooks don't seem to show up in the PR for some reason.

@robbieaverill any ideas?

[andrewandante]

Doesn't seem to like the wildcard when installed on PHP 5.6. Assuming we are only checking .php files we should adjust the phpcs call to:

if [[ $PHPCS_TEST ]]; then vendor/bin/phpcs --extensions=php src/ tests/ ./; fi

The default extension set is .inc, .php, .js and .css - so we could well leave it at that also.

[andrewandante]

Welp, well that ran phpcs over the whole framework etc. Given that there are no .php files in the module root, I'm going to just remove the ./ part.

Test test here: https://github.com/silverstripe/silverstripe-redirectedurls/pull/54

[dhensby]

ready for review

[robbieaverill]

Ok, there's a lot of duplication of effort for SS4 upgrades in this module:

• 52 - adds a new license file

• 49 - converts the image from png to svg

• 48 - closed, the others have surpassed it

Hopefully we can merge this then salvage some of the improvements in the other PRs separately

[andrewandante]

@robbieaverill have updated the PR with the LICENSE and redirect.svg files from @stevie-mayhew and @adrexia - thanks both!

[dhensby]

@andrewandante did you push them?

[andrewandante]

Definitely didn't forget and just push it now 😜

[dhensby]

I think for GitHub to recognise your LICENSE file it has to be LICENSE.md... (see: https://help.github.com/articles/licensing-a-repository/#determining-the-location-of-your-license)

[edlinklater]

@robbieaverill @dhensby Just as an FYI, recently while working on an SS4 project I wrote a new Redirector module using Middleware. Executes before the full Director chain and therefore is much more performant than using onBeforeHTTPError404. Would you consider a PR to change this module to use Middleware? https://github.com/edgarindustries/silverstripe-redirectmiddleware

[robbieaverill]

It sounds like a pretty good example of using middleware to achieve something, so if it will be more performant and still meet the same A/Cs then I don't see why not 😄 sounds like an API change to me, so if it's going to be done it would ideally be done before a SS4 stable tag.

[edlinklater]

@robbieaverill Sweet, I'll have a go at combining what I've done into this module.

[dhensby]

Moving to a middleware would be much better. TBH I didn't really look in any detail at the code and how it was written, I was just being a bit more mechanical about making it SS4 compat.

[andrewandante]

Think the best approach would be to merge this, tag some sort of alpha with the caveat that it will be moving to a middleware approach (so that people desperate to use it have something to work with) and then move the middleware work to another PR for merge before stable. I agree that it sounds like a much better approach @edlinklater , top job

[robbieaverill]

Over to you @dhensby!