We have to convert the module to short array syntax as per new coding standards.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | redirect-Convert_module_to_use_short_array_syntax-2961293-9-D8.patch | 73.65 KB | thalles |
| #6 | redirect-2961293-6.patch | 70.65 KB | daiwik.addweb |
| #2 | 2961293-2.patch | 73.01 KB | deepakkumar14 |
Comments
Comment #2
deepakkumar14 commentedSubmitting patch fro this.
Comment #3
nkoporecFound two missed array().
1. In RedirectAPITest.php
2. In RedirectCheckerTest.php
Comment #4
prashant.c@nkoporec
This is commented out code why do we need to make the changes in this ?
Comment #5
berdirThis has several conflicts now and needs a reroll, sorry for the delay.
Comment #6
daiwik.addweb commented@all Please find my patch where I resolved it from the first & worked on it as per the standard errors are shown in Pareview. kindly review it & let me know if you still face any challenges and issues.
Comment #7
daiwik.addweb commentedComment #9
thallesFollow the patch!
Comment #10
dwwThere are a few remaining references to
array(with patch #9 applied, but they're all in commented-out code. I guess we don't care about that, and hopefully we'll fix any remaining CS bugs as we touch those parts.This is blocking a lot of other things. It's a huge patch, and a beast to review. I just went over it all and didn't catch any unrelated or unintended changes. The only thing I saw that looked at all funny is this (from src/Form/RedirectForm.php):
I'm not totally sure that's legit code-style, but that's basically identical to what we have now, with only the array() vs. [] change. So if that's not legit, the code in Git isn't legit, either. So it's not a regression in code style because of this patch. If that needs further CS help, it should probably happen in a follow-up or as part of #2957751: Maintaining drupal coding standards.
Applies cleanly to the end of 8.x-1.x.
All tests pass with the patch applied.
Brief local click-testing seems to be fine, too.
Let's get this in so we don't keep running into people trying to fix any of this in unrelated patches (like at #2958635-6: Restore link to add a redirect on node edit forms).
Yes, it's going to make D7 patches no longer apply to D8. But I think the era of "easy" backporting/cherrypicking is probably over, and anything that touches D7 at this point will need to be rerolled for D8, anyway.
Given all of this, RTBC.
Thanks,
-Derek
Comment #12
berdir> Yes, it's going to make D7 patches no longer apply to D8. But I think the era of "easy" backporting/cherrypicking is probably over.
That era never existed, there's hardly a line of code the same as in the 7.x version. I don't care about 7.x but this will break a lot of other contrib patches, but there's not too much activity atm.
Comment #13
dwwThanks for getting this in!
I mostly don't, either, but I'm concerned about the state of this module. It seems the D7 and D8 versions have completely diverged, and no one is paying any attention to that fact. See for example #2220061-11: Add permission to restrict access to a user's own redirects only. Also, I just tried skimming #905914: Merge global redirect functions into Redirect module (linked from the project page) which is a scary and confusing read about yet more divergent functionality between the D7 and D8 versions. This seems to be evolving into quite a mess. Is there any plan to deal with this problem and bring some sanity and unification to this? Or should we fork the D8 project into a completely separate git repo and project? If the D7 and D8 branches are complete rewrites, maintained by different people, with different standards, and different visions, with diverging feature sets, why share a project page? It seems that's going to create more problems than it solves.
Thanks,
-Derek