Maintaining drupal coding standards

Assigning to myself as I'm triaging all RTBC issues.

Patch no longer applies (see below).

Although we should normally fix all PHPCS problems in one issue, given the very large patch and potential for it getting stale, it might make sense to pull some of these out and make this a META issue.

I wouldn't want too many separate issues though... maybe 4 max... find some of the larger problems that can have their own issues and then we can fix those first and have a patch that handles the remaining issues. Marking this postponed for now in case someone wants to make a child issue for the first big fix.

	modules/redirect_404/redirect_404.module.orig
	modules/redirect_404/src/Controller/Fix404IgnoreController.php.orig
	modules/redirect_404/src/EventSubscriber/Redirect404Subscriber.php.orig
	modules/redirect_404/src/EventSubscriber/Redirect404Subscriber.php.rej
	modules/redirect_404/src/Render/Redirect404LogSuppressor.php.rej
	modules/redirect_404/tests/src/Functional/Fix404RedirectUILanguageTest.php.orig
	modules/redirect_404/tests/src/Functional/Fix404RedirectUILanguageTest.php.rej
	modules/redirect_404/tests/src/Functional/Fix404RedirectUITest.php.orig
	modules/redirect_404/tests/src/Functional/Fix404RedirectUITest.php.rej
	modules/redirect_404/tests/src/Functional/Redirect404LogSuppressorTest.php.orig
	modules/redirect_404/tests/src/Functional/Redirect404TestBase.php.rej
	modules/redirect_domain/tests/src/FunctionalJavascript/RedirectDomainUITest.php.orig
	modules/redirect_domain/tests/src/Unit/DomainRedirectRequestSubscriberTest.php.orig
	modules/redirect_domain/tests/src/Unit/DomainRedirectRequestSubscriberTest.php.rej
	redirect.generate.inc.rej
	redirect.module.rej
	src/Plugin/Field/FieldWidget/RedirectSourceWidget.php.rej
	src/RedirectRepository.php.orig
	tests/src/Functional/GlobalRedirectTest.php.rej
	tests/src/Functional/RedirectUILanguageTest.php.orig
	tests/src/Functional/RedirectUILanguageTest.php.rej
	tests/src/Functional/RedirectUITest.php.rej
	tests/src/FunctionalJavascript/RedirectJavascriptTest.php.orig
	tests/src/FunctionalJavascript/RedirectJavascriptTest.php.rej
	tests/src/Kernel/Migrate/d6/PathRedirectTest.php.orig
	tests/src/Kernel/RedirectAPITest.php.orig
	tests/src/Kernel/RedirectAPITest.php.rej
	tests/src/Unit/RedirectCheckerTest.php.rej
	tests/src/Unit/RedirectRequestSubscriberTest.php.orig
	tests/src/Unit/RedirectRequestSubscriberTest.php.rej
	tests/src/Unit/RouteNormalizerRequestSubscriberTest.php.orig
	tests/src/Unit/RouteNormalizerRequestSubscriberTest.php.rej

Hi Kristen,

Thanks for looking at this.

In the interests of getting these coding standards fixes over the line now that the project has an active maintainer, I've re-rolled #29 along with some additional follow-ups that phpcs gave me. I have opted to ignore a small number of phpcs complaints -- a few cases of \Drupal calls should be avoided in classes, use dependency injection instead and unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option. both of which I deemed out of scope for this issue (they should be looked at in a follow-up issue).

Please review at: https://git.drupalcode.org/issue/redirect-2957751/-/commits/coding-stand...

There are two related commits which might be considered out of scope as well. The first was due to an apparent error in #29, and the second is related. I think I've done the right thing here, but it needs review.

* https://git.drupalcode.org/issue/redirect-2957751/-/commit/20a41463bbe4c...
* https://git.drupalcode.org/issue/redirect-2957751/-/commit/2de86d1ef5ce2...

It is quite time-consuming to re-roll such a large patch, so I'm hoping we can finally get this dealt with so that this is the last time it has to be done.

I'm not used to the MR workflow on d.o., but it seems as if "draft" MRs aren't (and can't be) tested, and fixup commits auto-trigger the change to "draft" (despite this MR being marked for eventual commit squashing); so I've squashed and rebased and force-pushed that, and set the MR back to "ready" in order to be able to re-test.

Tests have now passed.

Could this please be reviewed as a priority?

Per my comment in #35, it is really time-consuming to re-roll such a large patch. I spent the time on it in the hopes of seeing it merged at last, but I don't want to do it again. Please don't let it get stale again?

Nice work, only found a single thing to fix.

Thanks, merged.

Berdir → closed merge request !66

Automatically closed - issue fixed for 2 weeks with no activity.