Problem/Motivation

In #3166543: Deprecate UiHelperTrait::drupalPostForm, keep deprecation silenced and follow ups, drupalPostForm was deprecated and some conversions to submitForm done already.

In this issue, complete the conversion and remove the deprecation silencer.

Proposed resolution

First of all, given the size of the conversion, develop a script to automate as much as possible. Then, fix manually the leftovers.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3186661-6.patch1.03 MBpaulocs
#4 3186661-4.patch1.03 MBmondrake
#4 interdiff_2-4.txt3.26 KBmondrake
#2 3186661-2.patch1.03 MBmondrake
#2 convert.php_.txt2.42 KBmondrake

Issue fork drupal-3186661

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

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Active » Needs review
StatusFileSize
new convert.php_.txt2.42 KB
new 3186661-2.patch1.03 MB

The PHP script I developed for automated conversion, and an initial patch.

Status: Needs review » Needs work

The last submitted patch, 2: 3186661-2.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_2-4.txt3.26 KB
new 3186661-4.patch1.03 MB

Fixes and manually removing/adjusting deprecation stuff.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

The script looks good and the manual fixes are trivial. However, this already needs a reroll and will almost certainly need scheduling as it's going to be disruptive to basically every functional test patch currently in the queue.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
StatusFileSize
new 3186661-6.patch1.03 MB

Patch rerolled.

daffie’s picture

daffie commented
Title: Remove usage of drupalPostForm » [needs scheduling] Remove usage of drupalPostForm
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: [needs scheduling] Remove usage of drupalPostForm » [May 24, 2021] Remove usage of drupalPostForm

Scheduled for the middle of the beta window for 9.2.0. This gives time for 9.2.0-beta1 to bed in but get this committed before the freeze for 9.2.0-rc1.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3210939: [meta] Disruptive patches for 9.2 beta

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

Spokje made their first commit to this issue’s fork.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Postponed

To prevent re-rolling this huge patch every time a commit is made on 9.2.x-dev (since it touches so many test classes), let's wait until this one is up for actually being committed in #3210939: [meta] Disruptive patches for 9.2 beta.

Postponing until then.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Postponed » Needs review
Issue tags: -Needs reroll

Rerolled and pushed to the MR, with remaining fixes applied with some horrible regex

$ rg -l drupalPostForm|xargs sed -i -E 's/^( *)\$this->drupalPostForm\(([^,]+), /\1$this->drupalGet(\2);\n\1$this->submitForm(/'

longwave opened merge request !686

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

Needs a reroll.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje

Rerolling.

spokje’s picture

spokje
Primary language Dutch
commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Needs work » Needs review
spokje’s picture

spokje
Primary language Dutch
commented
Title: [May 24, 2021] Remove usage of drupalPostForm » [Patch to be ported to 9.1.x] [May 24, 2021] Remove usage of drupalPostForm
spokje’s picture

spokje
Primary language Dutch
commented
Title: [Patch to be ported to 9.1.x] [May 24, 2021] Remove usage of drupalPostForm » [May 24, 2021] Remove usage of drupalPostForm
Version: 9.1.x-dev » 9.2.x-dev

Argh, reverting changes in Title and Version

daffie’s picture

daffie commented
Assigned: spokje » daffie

I am reviewing.

daffie’s picture

daffie commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @daffie for his eagle-eyed review.

I can't resolve threads, since I'm not the original creator of the MR, but I've made the requested changes (https://git.drupalcode.org/project/drupal/-/merge_requests/686/diffs?dif...).

Also merged the latest HEAD from 9.2.x in.

Back to The Future NR.

daffie’s picture

daffie commented
Assigned: daffie » Unassigned
Status: Needs review » Needs work

This MR changes a lot of multi-line code to a single line version, which lowers the readability a lot. There a lot of places where this is happening. Please change them all back. Only for the ones that are now multi-line. I know that it a bit of work, sorry.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

Working on it

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review

I merged branch 9.2.x into this branch and added the suggested change made by @daffie.
Notice that I only added multi-line code where the array size is bigger than one or if the array index or value are big.

daffie’s picture

daffie commented
Status: Needs review » Needs work

Back to need work from the removal of the one line.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Addressed #29.

daffie’s picture

daffie commented

All changes in the MR from the module layout_builder and beyond are for me RTBC.
I will review the rest tomorrow.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
The deprecation message supression has been removed.
There is a deprecation message test.
For me it is RTBC.

daffie’s picture

daffie commented
Status: Reviewed & tested by the community » Needs work

The MR needs to be rebased.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje

Working on this.

Spokje opened merge request !706

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Found one double use of drupalGet in the 9.2.x MR, fixed that in both MRs.

9.2.x: MR686
9.3.x: MR706

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

The MR with id 686 is for me RTBC.

daffie’s picture

daffie commented

The result of running the command diff 686.diff 706.diff is empty. Therefore both patch files are the same. For me is the MR 706 also RTBC.

  • catch committed 3066423 on 9.3.x 
    Issue #3186661 by Spokje, longwave, mondrake, paulocs, daffie: [May 24,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Spokje opened merge request !709

catch’s picture

catch
he/him
Primary language English
commented
Title: [May 24, 2021] Remove usage of drupalPostForm » [backport] [May 24, 2021] Remove usage of drupalPostForm
Version: 9.2.x-dev » 9.1.x-dev
Status: Fixed » Needs review

Spokje closed merge request !706

spokje’s picture

spokje
Primary language Dutch
commented

All green now.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs review » Reviewed & tested by the community

All drupalPostForm calls were properly replaced and looks good to me.
All calls to $this->submitForm keeps multi-line when the array size is bigger the one.

catch’s picture

catch
he/him
Primary language English
commented
Title: [backport] [May 24, 2021] Remove usage of drupalPostForm » Remove usage of drupalPostForm
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.1.x, thanks!

Spokje closed merge request !709

  • catch committed e6e52aa on 9.1.x 
    Issue #3186661 by Spokje, longwave, mondrake, paulocs, daffie, catch:...

Status: Fixed » Closed (fixed)

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