Convert to OOP Hooks

nicxvan → changed the visibility of the branch 3487957-convert-to-oop to hidden.

For some reason some use statements are being duplicated.

nicxvan → changed the visibility of the branch 3487957-convert2.0 to hidden.

Set to needs review for discussing the process with the maintainers.

I think we need an MR to 6.3.x that sees what tests are failing. Depending on the number of tests failing we could commit this to 6.3.x or wait until 6.4.x.

I can work on this again, the main thing is I wanted to confirm if you were willing to help track down the issues and do a single merge.

It might be a couple of days though.

Let's see what the issues are before we manually fix them. I am assuming you are using Drupal Rector.

Yes, more specifically the script here: https://gitlab.com/-/snippets/4769802

I've done no code sniffing or manual changes, here is just running rector.
https://git.drupalcode.org/project/webform/-/merge_requests/582/diffs

I think I have a fix for the comments being deleted thanks to @chx

nicxvan → changed the visibility of the branch 3487957-print to hidden.

nicxvan → changed the visibility of the branch 3487957-rectorOnly to hidden.

Steps I took for rectorv3

Checkout that branch in an 11.x version of drupal in ddev
Add this script https://gitlab.com/-/snippets/4769802 to ddev and run it
yes to both questions
Commit rector changes

Find and replace // phpcs:ignore with something easily found (since it breaks codefix) I used // elephant
Run codefix in ddev

./vendor/bin/phpcbf \
  --standard="Drupal,DrupalPractice" -n \
  --extensions="php,module,inc,install,test,profile,theme" \
  web/modules/contrib/webform

Find and replace // elephant for // phpcs:ignore to swap it back
Commit and push

3 known codesniff issues
1. unused use statements, rector expands arguments - not sure why codefix isn't fixing these
2. t() in classes
3. \Drupal in classes, rector doesn't do injection

nicxvan → changed the visibility of the branch 3487957-rectorv2 to hidden.

nicxvan → changed the visibility of the branch 6.3.x to hidden.

rectorv4 is the MR to review just waiting on tests. I updated the script to handle the unused use statements too.

nicxvan → changed the visibility of the branch 3487957-rectorv3 to hidden.

This looks very promising. If you are able to get an MR that has all the tests passing and phpcs is okay(ish). We should make the commit.

Things like dependency injection can be fixed later (or never).

Sounds great! The only failures I see other than code sniffing are deprecations that are on HEAD so I assume those can be ignored.

I can look at all of the code sniff failures besides t() and dependency injection.

I can create a manual patch for those I think this week.

The phpcs checks were passing two days ago on 6.3.x, so they ought to keep passing.

The issue is twofold.

t() in .module doesn't throw phpcs errors but it does in classes.

Drupal service calls are the same.

Rector doesn't address these so they have to be manually addressed.

I need to still review the other cs failures.

Would you be willing to having a temporary skip rule for those two?

I spent a couple of hours going through, still got a lot to go.

I'd like confirmation on whether we can skip the t() and injections.

I'm inclined that we should not go backwards on our coding standards compliance.

Why the new merge request? What is different?

I am testing a different tool for addressing the arrays.

As I mentioned it's not going backwards.

These are patterns that exist in the procedural code but get flagged in classes.

What I mean by going backwards is not that the code is getting worse (it's not). I just mean that currently phpcs passes and with this merge request it doesn't.

nicxvan → changed the visibility of the branch rector4b to hidden.

I just noticed we already have an ignore rule for \Drupal calls, it's just not taking effect, let me look at that. If that were in place this would be far more manageable.

Just to add some context here, this is a bulk rector pass, conflicts for auto generated commits like this need to be just scrapped and redone, they are not like normal conflicts where you can resolve them since it's bulk copying functions to methods. The more manual work on top of that the more fragile it becomes.

I'm willing to commit to resolving these codesniff rules in follow ups, they are generally not complex to fix and once this is in those issues could be rebased with a normal workflow.

The risk of conflict is much lower.

CS is passing now with rules addressing t() \Drupal and one DateFormat calls.

I completely understand that we must skip fixing the dependency injections.

If you are saying in a later commit, we can automatically fix the t() calls since all that is required is the trait and then a search and replace, this is good enough to be committed.

Neither of those will be fixed automatically, but I am committing to working on both of those follow ups if this gets committed here.

The thing is I can manage and rebase those two fixes in a follow up (or more likely separate followups) in a more standard manner once this is in.

This is a very big patch and I am feeling some pressure to have full release for 6.3.0. Perhaps we should put this onto a 6.4.x branch.

While it is a big patch, this is the same process used for core to convert, and we can spot check that the conversions are accurate. Since this is rector it's really just copying the hooks to a hook class an method.

Webform also has more tests than most modules so we can have more confidence than most modules would have that this won't introduce systemic issues.