Problem/Motivation

In #3323994: Fix PHPStan L1 errors "Constructor of class Foo has an unused parameter $bar" we discovered the $language_manager argument in the constructor of \Drupal\menu_link_content\Form\MenuLinkContentForm is unused.

Let's deprecate it in 10.1 and remove it in 11.0.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3325175-8.patch5.76 KBmondrake

Issue fork drupal-3325175

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

Spokje created an issue. See original summary.

Spokje opened merge request !3066

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: +PHPStan-1
spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review

Note: This deprecation was not done according to the documentation here: https://www.drupal.org/about/core/policies/core-change-policies/drupal-d..., but according to this comment by @alexpott here: https://git.drupalcode.org/project/drupal/-/merge_requests/3008#note_135753

If this is the accepted new way of deprecation, we need to update the mentioned documentation.

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

smustgrave’s picture

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

Reviewed the changes and they look good and I see they were green before comment #4

#5 appears to be a rebasing.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Would it make sense to use union types instead of removing the typehints completely? I will give a try.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3325175-8.patch5.76 KB

This is the idea from #7 in a patch.

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented

Hmm, that does indeed work, although I must say this looks really strange IMO. Not sure i prefer this over losing the type hinting?

xjm’s picture

xjm
she/her
Primary language English
commented

I think the union types are a good choice now that we can allow them.

quietone’s picture

quietone commented

Setting to NR because of the new approach.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Moving to NW for the open thread.

spokje’s picture

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

spokje
Primary language Dutch
commented

Sadly we seem not ready for these kind of changes within a time-frame that's less than one year.

So let's stick with the MR and the official ways.

mondrake’s picture

mondrake
Location 🇮🇹
commented

I think now the reference for this type of dances should be #3323209: Deprecate calling Drupal\block\Plugin\migrate\process\BlockTheme::_construct() with the $migration argument. In particular, adding typehinting to the class properties would ease the check that each property gets the expected type.

spokje’s picture

spokje
Primary language Dutch
commented

I fully agree with @mondrake, but sadly this isn't
The format to use here is not on the deprecation policy page..

No matter how much I would like to use the new format, I'm a bit tired of doing do and then having to change everything back to the official way.

So I've gone the official wsy to begin with now.
Let's hope for some official code style changes soon.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

But that HAS changed since #9, hasn't it? https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...

And #3323209: Deprecate calling Drupal\block\Plugin\migrate\process\BlockTheme::_construct() with the $migration argument WAS committed.

Anyway, let's RTBC this, and see committers' feedback.

spokje’s picture

spokje
Primary language Dutch
commented

AFAICT I (tried to) applied the new format. Although the example with only 3 arguments and deprecating the 2nd is a bit sparse for our 6, deprecate 3rd real life situation.

spokje’s picture

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

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

3 points. The first one is a do-do imho.

spokje’s picture

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

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned

@mondrake: Resolved one thread, don't think we can resolve a second one and don't understand the third one.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work

Two birds with one stone, I think if we fix the 3rd we also implicitly fix the 2nd, see inline

spokje’s picture

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

Thanks @mondrake, should have figured that out myself.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Few nits in inline comments.

smustgrave’s picture

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

Change looks good to me.
All threads resolved.
Lets see if this can make it for 10.1

andypost’s picture

andypost
he/him
Primary language Russian
commented

I bet it needs DeprecatedServicePropertyTrait to be added for removed service

spokje’s picture

spokje
Primary language Dutch
commented

Thanks @andypost, never knew about the existence of DeprecatedServicePropertyTrait
#TIL

But I don't think it's needed here, since the $language_manager argument (interestingly) never was a property on the MenuLinkContentForm class?

andypost’s picture

andypost
he/him
Primary language Russian
commented

As I see there's few contrib modules extending the class, so would be great to provide BC
ref http://codcontrib.hank.vps-private.net/search?text=MenuLinkContentForm&f...

spokje’s picture

spokje
Primary language Dutch
commented
Status: Reviewed & tested by the community » Needs review

Added DeprecatedServicePropertyTrait

andypost’s picture

andypost
he/him
Primary language Russian
commented

Added suggestion as expected property is languageManager but service name language_manager

spokje’s picture

spokje
Primary language Dutch
commented

Applied suggestion, closed all open threads.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

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.

spokje’s picture

spokje
Primary language Dutch
commented

Based MR on 11.x

  • catch committed d704aa38 on 11.x 
    Issue #3325175 by Spokje, mondrake, andypost, xjm: Deprecate calling \...
catch’s picture

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

Looks good. Committed/pushed to 11.x, thanks! I updated the deprecation message on commit.

Spokje closed merge request !3066

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

Published change record.