Problem/Motivation

Core supports OOP hooks
We can automate webform

Steps to reproduce

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

ddev composer require --dev friendsofphp/php-cs-fixer

#!/bin/bash

## Description: Sniff code
## Usage: cdu
## Example: cdu

./vendor/bin/php-cs-fixer fix \
  web/modules/contrib/webform \
  --rules=no_unused_imports

Commit and push

Proposed resolution

Approach 1
Run rector
Check tests
Add patches to fix any testing / functional issues
Pick date for merge
Merge

Remaining tasks

Questions for maintainers

  • Are you willing to commit a conversion in one issue, it can be less disruptive for a one time conversion like we achieved with core.
  • If you prefer several smaller issues how would you like to break this up.
  • I'm willing to do the legwork on rector and patching, would you be able to help track down some test failures

Other notes, when there are a lot of failures like in the latest test run it's usually something that can be fixed with Rector, but I want to settle on a process before investing too much time on this.

Review process
Review conversion
Review any test failures
Find out why LegacyHook is being reported as missing

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Issue fork webform-3487957

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan opened merge request !548

nicxvan opened merge request !549

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

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented

For some reason some use statements are being duplicated.

nicxvan’s picture

nicxvan commented
Title: Convert to OOP Hooks » Convert webform to OOP Hooks

nicxvan opened merge request !550

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

nicxvan’s picture

nicxvan commented
Title: Convert webform to OOP Hooks » Discuss process for converting webform to OOP Hooks
Issue summary: View changes
Status: Active » Needs review
nicxvan’s picture

nicxvan commented

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

nicxvan’s picture

nicxvan commented
Issue summary: View changes
jrockowitz’s picture

jrockowitz commented

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.

nicxvan’s picture

nicxvan commented

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.

jrockowitz’s picture

jrockowitz commented

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

nicxvan’s picture

nicxvan commented

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

nicxvan opened merge request !582

nicxvan’s picture

nicxvan commented

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

nicxvan’s picture

nicxvan commented

I think I have a fix for the comments being deleted thanks to @ghost of drupal past

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

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

nicxvan opened merge request !583

nicxvan opened merge request !584

nicxvan’s picture

nicxvan commented

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’s picture

nicxvan commented
Issue summary: View changes

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

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

nicxvan opened merge request !589

nicxvan’s picture

nicxvan commented

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.

jrockowitz’s picture

jrockowitz commented

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).

nicxvan’s picture

nicxvan commented

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.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

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

nicxvan’s picture

nicxvan commented

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?

nicxvan’s picture

nicxvan commented

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.

nicxvan opened merge request !590

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

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

Why the new merge request? What is different?

nicxvan’s picture

nicxvan commented

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.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

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.

nicxvan’s picture

nicxvan commented

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.

nicxvan’s picture

nicxvan commented

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.

jrockowitz’s picture

jrockowitz commented

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.

nicxvan’s picture

nicxvan commented

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.

Created:
#3500968: Address t() in new hook classes so they use StringTranslationTrait
#3500969: Add Dependency injection to Hook classes

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

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.

nicxvan’s picture

nicxvan commented

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.

nicxvan’s picture

nicxvan commented

Ok code sniff is fixed and I fixed all php stan issues for the new hook classes (Except one for bootstrap because bootstrap isn't installed in that case)

This should be ready to go.

jrockowitz’s picture

jrockowitz commented

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.

I completely agree with you that this should not introduce systemic issues and merging it would nudge more contrib modules to use OOP hooks.

nicxvan’s picture

nicxvan commented

I checked the merge and saw no changes to the hooks, I pushed a fix for the sniff errors.

  • jrockowitz committed cdc9dfaa on 6.3.x authored by nicxvan 
    Issue #3487957: Discuss process for converting webform to OOP Hooks
jrockowitz’s picture

jrockowitz commented
Status: Needs review » Fixed

Your persistence has paid off. I am at DrupalCon is time to merge this. Thank you!!!

nicxvan’s picture

nicxvan commented

Thanks! Glad I could help!

nicxvan closed merge request !590

nicxvan closed merge request !584

nicxvan closed merge request !583

nicxvan closed merge request !582

nicxvan closed merge request !550

nicxvan closed merge request !549

nicxvan closed merge request !548

Status: Fixed » Closed (fixed)

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