Problem/Motivation

Smart Trim has a dependency on Token Filter. It tests against next major, but composer is currently failing for this because there isn't a D12 compatible version of Token Filter.

Creating issue so I can use patch file with composer patches plugin.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork token_filter-3593924

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

lostcarpark created an issue. See original summary.

lostcarpark opened merge request !24

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

Sure we can start this now... this is the first time I'm approaching 12 - what do you reckon the upgrade status module means by "unsafe new static()"? That's how we used to do DI, I guess it must have changed?

lostcarpark’s picture

lostcarpark commented

I believe the reason why new static() is considered unsafe is because if another class inherits this one, and adds to the constructor, but doesn't also override the create() function, it will fail.

One option is to mark the class "final", which will prevent it from being inherited.

A less drastic option is to replace new static() with new self(). This will mean that create() will always create an instance of the class it's implemented in, which means an inherited class will always need to override it.

In the longer term, autowiring will allow create() functions to disappear altogether. Plugins support this from Drupal 11.3, so you could just delete create() functions and it will work anything above that. However, if you want to continue supporting D10, probably should keep them for now.

Happy to change to use new self() if you would like.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

Thanks for the explanation 🙏

I do want to keep supporting D10 while it is still in support, so yes please go ahead with new self() thank you 🙇‍♂️

lostcarpark’s picture

lostcarpark commented

I don't think new self() will work for plugins, because it doesn't match the function prototype.

So I think the options are mark the classes final (and if we do that, we need to remove from ignoreErrors in phpstan.neon, or we could just acknowledge the Upgrade Status findings and allow it to fail.

I'm not sure if there's a way to tell Upgrade Status to ignore an error.

lostcarpark’s picture

lostcarpark commented

Gabor linked me to this issue: #3579468: Module reports phpstan "Unsafe usage of new static()" errors

It seems that Upgrade Status will no longer warn about unsafe use of new static, so this warning will go away once Gitlab CI updates to the new version.

I would be inclined to leave the new static() calls alone in light of this.

lostcarpark’s picture

lostcarpark commented
Status: Active » Needs review

Reverted back to plain new static().

Everything currently passing except Upgrade Status, but hopefully when Gitlab CI updates to the latest version, that will also pass.

Note that "phpstan (previous major)" validates attributes, but D10 doesn't know about many of the D11+ attributes, so I use a sed script to remove them for that task only.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

Yeah cool, thanks very much for looking into it! Good to know the outcome, very happy to merge with that evidence.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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