Problem/Motivation

Follow-up to #3117553: Change UI for /admin/reports/updates/update to not mention "Enabled/Disabled" modules and themes

dww: The scope creeper in me would want to change variable names and other internal code to match. E.g. stuff like this:

-            $projects['disabled'][$name] = $entry;
+            $projects['uninstalled'][$name] = $entry;

Would that be viable, while we're at it?

xjm:

String changes are also backportable during beta, but form structure changes aren't. So let's keep it to strings.

So here's a follow-up to remove the technical debt of the form structure, variable names, etc.

Proposed resolution

Make sure none of the implementation details of UpdateFormUpdateManager mention 'disabled', either.

Remaining tasks

  1. Do it.
  2. Make sure we didn't break anything.
  3. Reviews/improvements.
  4. RTBC.
  5. Commit.

User interface changes

None.

API changes

TBD.

Data model changes

None.

Release notes snippet

TBD.

CommentFileSizeAuthor
#16 interdiff_14-16.txt1.92 KBnarendrar
#16 3121870-16.patch7.59 KBnarendrar
#14 interdiff_9-14.txt994 bytesnarendrar
#14 3121870-14.patch5.3 KBnarendrar
#9 3121870-9.patch5.29 KBquietone

Issue fork drupal-3121870

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

dww created an issue. See original summary.

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

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.

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

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Parent issue: #3117553: Change UI for /admin/reports/updates/update to not mention "Enabled/Disabled" modules and themes » #2338175: [meta] Update module terminology still refers to disabled extensions when it should be uninstalled
quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 3121870-9.patch5.29 KB

This makes the suggested changes. It also includes changing some comments.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

believe these should be changed too

'#prefix' => '<h2>' . $this->t('Disabled') . '</h2>',

$form['projects']['#prefix'] = '<h2>' . $this->t('Enabled') . '</h2>';

quietone’s picture

quietone commented
Status: Needs work » Needs review

The changes asked for in #10 are for the UI. This issue is for the implementation details, the UI is done in a sibling issue.

smustgrave’s picture

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

In that case think this is good.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs reroll now that #3117553: Change UI for /admin/reports/updates/update to not mention "Enabled/Disabled" modules and themes landed.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3121870-14.patch5.3 KB
new interdiff_9-14.txt994 bytes

Status: Needs review » Needs work

The last submitted patch, 14: 3121870-14.patch, failed testing. View results

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
StatusFileSize
new 3121870-16.patch7.59 KB
new interdiff_14-16.txt1.92 KB
narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
dww’s picture

dww
we/he/they
commented
Issue tags: +Needs followup

Thanks for working on this! I had forgotten this issue existed.

dww%  egrep -ir disabl core/modules/update/src/Form |  wc
      15      87    1638
dww% git apply 3121870-16.patch 
dww% egrep -ir disabl core/modules/update/src/Form | wc

So that's good. 😅

Sadly, there are still a ton of references outside of that directory:

dww% egrep -ir disabl core/modules/update | wc
      52     684    7708

We'll need another child (or children?) of #2338175: [meta] Update module terminology still refers to disabled extensions when it should be uninstalled for that...

I think the patch looks okay, but I need to do a more careful review of the surrounding code to make sure we're only changing internal stuff here, not the structure of any monster ArrayPI arrays.

quietone’s picture

quietone commented
Issue tags: -Needs followup

I think it would be better to have followups made as a child of the parent so a followup is not considered a step before this is committed. I am removing the tag. I've updated the IS in the parent to be clear that two of the remaining tasks need issues made.

I looked at the changes in #14 and #15, which I agree with. But I can't RTBC this issue.

smustgrave’s picture

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

But I can! haha

quietone’s picture

quietone commented
Title: Change implementation details of UpdateManagerUpdateForm to not mention 'disabled' » Change implementation details of UpdateManagerUpdateForm to use install/uninstall

This is actually changing both enable and disable, so updating title.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3121870-16.patch, failed testing. View results

smustgrave’s picture

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

Rerunning but believe to be unrelated.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3121870-16.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review

Unrelated failure, retesting

1) Drupal\Tests\Component\Utility\RandomTest::testRandomMachineNamesUniqueness
RuntimeException: Unable to generate a unique random machine name
quietone’s picture

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

Restoring RTBC

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

quietone opened merge request !5525

quietone’s picture

quietone commented
Status: Needs work » Needs review

Converted to an MR

smustgrave’s picture

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

Hiding patches for clarity.

Was previously RTBC so restoring status.

longwave’s picture

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

Committed b102e5e and pushed to 11.x. Thanks!

Not eligible for backport due to user facing string changes and some internal API changes that, while unlikely to affect anyone, we don't want to drop into a patch release.

  • longwave committed b102e5e9 on 11.x 
    Issue #3121870 by quietone, narendraR, smustgrave, dww: Change...

quietone closed merge request !5525

Status: Fixed » Closed (fixed)

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