search

md-systems / redirect

DEPRECATED: Redirect moved back to drupal.org, use the official repository.
https://www.drupal.org/project/redirect
21 stars 29 forks source link

Don't allow redirects source to start with / #93

Closed googletorp closed 8 years ago

googletorp commented 8 years ago

Improve the UX by catching the most obvious error - creating redirects starting with / which is how you do it almost all other places in Drupal 8.

Berdir commented 8 years ago

Hm, looks like the subdirectory tests failed, currently re-testing https://travis-ci.org/md-systems/redirect/builds/108652688 to see if that's a problem with the change or not.

googletorp commented 8 years ago

What's the difference between the the two tests?

Berdir commented 8 years ago

It installs and runs the tests against drupal in a subfolder, e.g. http://localhost/subfolder. We're doing both because a) drupal.org testbot does it with a subfolder and b) we've had bugs/problems in the past with this.

googletorp commented 8 years ago

I looked a bit into this. I think your travis integration is fundamentally flawed.

I downloaded drupal 8.0.5 and made it available on localhost/drupal8, I'm using PHP 7 RC 8

I tried running the tests via interface - all passes I tried running the test with php core/scripts/run-tests.sh --module redirect This gave me the same failures as we are seeing here. I tried running the test with php core/scripts/run-tests.sh --module redirect --uri http://localhost/drupal8 all passes

Probably the issue is that the way the tests is run, it tries to access the wrong urls when logging in and creating stuff via the interface. Since the tests doesn't make any tests on the interface feedback, it simply fails when the redirects aren't created as expected.

Long story short - I think the code is fine and your testing setup is buggy.

Berdir commented 8 years ago

That is possible, it certainly doesn't block this PR since we can reproduce without it too. A quick test would be nice anyway, though :)

That said, this setup worked until ~1 month ago, so something must have changed.

googletorp commented 8 years ago

I added a test case for the validation added here.

Berdir commented 8 years ago

Thanks, merged.