Problem/Motivation

core/lib/Drupal/Core/Render/Element/RenderElement.php

This looks like a Drupal 7 deprecation.

      // @todo Legacy support. Remove in Drupal 8.
      if (isset($settings['method']) && $settings['method'] == 'replace') {
        $settings['method'] = 'replaceWith';
      }

Steps to reproduce

Proposed resolution

Remaining tasks

  1. Publish a change record (CR).

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#26 3303557-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3303557

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

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Parent issue: » #3301206: [meta] Fix incomplete/incorrect deprecations in Drupal core

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 made their first commit to this issue’s fork.

quietone opened merge request !5384

Tom Konda made their first commit to this issue’s fork.

tom konda’s picture

tom konda
Primary language Japanese
Location Kanagawa, Japan
commented
Status: Active » Needs work

I changed existing 'method' => 'replace' to 'method' => 'replaceWith' in test modules.
But core/lib/Drupal/Core/Render/Element/RenderElement.php has conflict, so need to resolve.

Binoli Lalani made their first commit to this issue’s fork.

Binoli Lalani opened merge request !7364

Binoli Lalani changed the visibility of the branch 3303557-deprecate-and-remove to hidden.

binoli lalani’s picture

binoli lalani commented
Status: Needs work » Needs review

Hello,

I have tried to resolve the conflicts of 5384 MR but It has unwanted code changes so I hide that MR and created a new MR. Please review it.

Thank You!

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Discussed with @smustgrave in slack, we should add a deprecation in 10.3.0 - can be in a separate MR against 10.3.x since the 11.x one seems fine.

Binoli Lalani opened merge request !7388

binoli lalani’s picture

binoli lalani commented
Status: Needs work » Needs review

Hello @catch,

Thank you for reviewing the MR. I have created another MR against 10.3.x branch and added deprecation.
Pipeline is failed due to phpstan error in system.module. I verified that it is not related to this code change. It is already there in the latest code.

Please review the MR and let me know if any changes needed.

Thank you!

catch’s picture

catch
he/him
Primary language English
commented

This looks fine. The 10.3.x phpstan fail is indeed in HEAD and should go away when fixed, although will need to rerun the pipeline or rebase to get a full test run.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

10.3 appearing green now, deprecation looks good.

  • nod_ committed 55339845 on 11.x 
    Issue #3303557 by Binoli Lalani, quietone, catch, Tom Konda, smustgrave...

  • nod_ committed ef97c53b on 10.3.x 
    Issue #3303557 by Binoli Lalani, quietone, catch, Tom Konda, smustgrave...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 5533984 and pushed to 11.x. Thanks!

nod_ closed merge request !7364

nod_ closed merge request !7388

Status: Fixed » Closed (fixed)

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

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Closed (fixed) » Needs work
Issue tags: +Needs change record

This issue came up in Slack: https://drupal.slack.com/archives/C03L6441E1W/p1716058078575899 and https://drupal.slack.com/archives/C03L6441E1W/p1716060358368069. IIUC, the pathauto module was still using 'method' => 'replace', at least once.

I think we should have a change record for this issue, so I am reopening it and adding the issue tag.

pandaski’s picture

pandaski commented

A change record will be helpful because I've identified that more modules could be affected:

search_api
panels
search_api_solr
pathauto
facets
search_api_attachments
diff
password_policy
shield

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue summary: View changes
Status: Needs work » Needs review

I added the draft change record (CR) https://www.drupal.org/node/3450770. I am moving this issue to NR for that.

If the CR looks good, then we can publish it and return this issue to Fixed.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3303557-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Fixed
Issue tags: -Needs change record

The change record looks good. The only comment I thought about making is that because replaceWith is the default method (as defined in Drupal.Ajax) it would also be possible to just remove the command altogether from the array. But I don't think that's as clear/easy to explain in a CR as using replaceWith explicitly.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue summary: View changes

Is it worth making a follow-up MR to reference the change record, not this issue, in the deprecation message?

catch’s picture

catch
he/him
Primary language English
commented

@benjifisher yes that wouldn't hurt.

benjifisher changed the visibility of the branch 3303557-reference-change-record to hidden.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Fixed » Needs review

Back to NR for https://git.drupalcode.org/project/drupal/-/merge_requests/8250

The deprecated code is already removed in the 11.x branch, so this follow-up is needed on the 10.3.x branch only.

benjifisher changed the visibility of the branch 10.3.x to hidden.

benjifisher opened merge request !8250

smustgrave’s picture

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

CR reads fine. Before/after examples are perfect.

  • catch committed a252954d on 10.3.x 
    Issue #3303557 by benjifisher, Binoli Lalani, quietone, catch, Tom Konda...

  • catch committed b68ac6a5 on 10.4.x 
    Issue #3303557 by benjifisher, Binoli Lalani, quietone, catch, Tom Konda...
catch’s picture

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

Committed/pushed to 10.4.x and cherry-picked to 10.3.x, thanks!

catch closed merge request !8250

Status: Fixed » Closed (fixed)

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

mxh’s picture

mxh commented

So many modules... Can this change be added to the automated D11 compatibility bot?

quietone closed merge request !5384