GitLab CI - PHPStan - "Unsafe usage of new static()"

All tests now passing!

I've done a bit of a deep dive into this particular PHPStan error (blog post coming soon) and I'm not crazy about ignoring errors of this type (or pretty much any other code quality issues).

Some alternatives:

• Make the SmartTrimFormatter class as final - this would mean that no one would be able to extend it - probably a bridge too far.
• Change return new static to return new self - this would mean that anyone who wants to to extend the class and override the constructor would also have to override the create() method to match. Since we don't know if anyone who is currently using this module is already overriding this class, we could potentially break some sites.
• Mark the constructor as final - this would allow folks to extend the class but not change the constructor - which could also break existing sites that have already extended this class and changed the constructor.

As a first pass, I'd love to hear if anyone is extending the SmartTrimFormatter class.

-mike

I think all of the currently available solutions restrict the ability to extend the class. I think that if we're okay with that, we should go all out and make the class final. I think that would be preferable to letting someone try to derive the class, only to find they got stuck when they needed to add a new dependency.

I suspect the most likely place that a class is likely to want to extend SmartTrimFormatter is in the module itself, which we are definitely not doing at the moment, or in companion modules, of which I'm not aware of any currently. However, it's possible someone is extending in a custom module.

I have fixed the issues and added a patch, please review
Thanks

Thanks @viren18febS,

I have reservations about changing new static() to new self() and making the class final. While it does fix the error, it also prevents the class from being extended.

One concern is that if sites are already using SmartTrimFormatter as a base class, they would be broken by this change. I'm not sure how likely that is. It would be possible to customize the behavior of Smart Trim by subclassing the filter, but it would seem a less obvious solution than forking the whole module.

If decide there is no good reason to subclass SmartTrimFormatter, then I'd be happy to go down this route, but I feel more discussion is needed first.

Ignoring the error message seem to me to be the correct fix in the Drupal context. It is understood that if __construct() changes in an extending class, create() needs to be changed to match.

However, this seems like it should be fixed globally for all projects instead of requiring each project to set their own ignoreErrors.

Thanks @lostcarpark and @Liam Morland for the MR/suggestion that ignores this issue, but as @lostcarpark knows, I'm (really) not a big fan of ignoring PhpStan issues.

@viren18febS thanks for your patch, but adding final to the class will potentially break the module for some users. Also - switching to self leads to another set of issues.

I did a bit of a deep dive into this particular PhpStan issue (see https://www.drupaleasy.com/blogs/ultimike/2023/12/how-best-handle-unsafe...) and I believe that adding final to the constructor is the way to go.

So, it would be great if someone wants to update the MR (no patches, please) to do two things:

• Add final to the constructor.
• Add a new "Extending Smart Trim" page to the documentation → and include/explain the code snippet from my blog post on how classes that extend SmartTrimFormatter can craft their create() method to allow new dependencies.

Sound like a plan?

thanks,
-mike

Hi Mike (@ultimike),

Thanks a lot for your great help on this issue and for the great blog post.

After triggering a build on one of the modules I maintain, I encountered the same issue and after doing a bit of search online, stumbled upon this ticket, with your last comment, which led me to your blog post, the documentation reference on DO and Phil Norton's blog post ( @philipnorton42 → ), with additional comments from Ekes ( @ekes → ), pointing to the Search API → module.

I started by testing/applying on our module your solution suggested at #9, as an attempt to fix related issue 📌 GitLab CI - PHPStan - "Unsafe usage of new static()" Fixed (with the same problem), which seemed to work quite well (bringing the tests back to green) and gave me the opportunity to better understand your approach so I could write some documentation.

Based on your instructions in your last comment, I went ahead and made the requested changes at #11 which are now ready to be reviewed in the following merge request:
https://git.drupalcode.org/project/smart_trim/-/merge_requests/78
The tests seem to have all gone back to green as well ✅:
https://git.drupalcode.org/issue/smart_trim-3405618/-/pipelines/72899

Additionally, as requested, I've ported the documentation I added for the Image Link Formatter module to Smart Trim's documentation guide and the page is now ready to be reviewed at:
https://www.drupal.org/docs/contributed-modules/smart-trim/extending-sma... →

The first paragraph and pattern example is taken from your blog post, as requested.
The additional paragraph with examples of the "get[SERVICE]/set[SERVICE]" pattern are taken from the Search API → module, based on Ekes' suggestions on Phil's blog post, which I have detailed a bit in related ticket ( #3412951 📌 GitLab CI - PHPStan - "Unsafe usage of new static()" Fixed ) with direct links to pieces of code in module's repository.
 

We would greatly appreciate if you could please try testing/reviewing the changes in the MR and more particularly give us your feedback on the documentation with code examples.

Once again, thank you very much for analyzing, documenting and gathering very helpful resources on this issue without which I certainly wouldn't have been able to find an appropriate solution for our module.

Feel free to let me know at any point if you have any questions, concerns, comments or suggestions on the changes from the MR or the added documentation page, I would surely be very happy to help.
Thanks in advance for your feedback and reviews!

DYdave → changed the visibility of the branch 3405618-phpstan.neon to hidden.

I've reviewed the change, and it looks good to me.

Documentation page is a great start. There are probably a few details that could be expanded upon. The examples don't show how currentUser is defined, but if it is protected or private, the first example would not allow unit tests that mock the dependencies, the second would, so it might be worth including a mention of that.

Setting to RTBC.

@DYDave - thanks so much!

The documentation page looks mostly good, but I did make some minor changes to clarify why we added final.

As soon as I figure out how to mark the new documentation page as "reviewed", It'll appear in the Smart Trim documentation menu.

-mike

I had a similar problem with the documentation page for one of my modules. I asked one of the documentation team to help, and they fixed it for me, but I still don't know how to do it.

Thanks a lot James (@lostcarpark) for your reviews, comments and feedback, it's super helpful!

Thank you very much Mike (@ultimike) for the speedy and very kind reply!

Glad to see the patch could be accepted (thanks for merging it Mike!) and the initial draft of the documentation is going in the right direction.

I'll let you both make the necessary changes to the doc page and keep an eye on it for any additional updates you would make so I could port them back to my other module to have a more complete documentation as well.

Mike, I think it's the first time I have the chance to cross your path and interact in tickets, so I'd like to take the opportunity to let you know I've been a big fan of yours and you've been on my Mentors list since 2018, when I bought one of your books for the first time: Development with DDEV (super well written to get started with DDEV).
I'm particularly grateful for everything you've been doing for our Drupal community, all your contributions, great blogs (big fan of your posts on DrupalEasy), presentations, etc...

Hopefully, I'll be able to meet you in person some day, if you get the chance to come to one of our next DrupalCons in Europe, perhaps in September in Barcelona and would love to attend one of your presentations, if you submit a session.
I spoke a bit with AmyJune (@volkswagenchick) at DC Lille last October (2023) who seems to also be a maintainer of the Smart Trim module, so it really looks like the module is in great hands :)

In any case, feel free to let me know at any time if you need any help with the module, or any other tickets (docs/tests/patches, etc...) in general, I would definitely be glad to participate, it's always a very fruitful learning experience.

Thanks again James and Mike for your great reactivity and feedback.
Cheers!

added documentation page to the menu. Thanks

@DYDave - wow, thanks for your kind words. Especially the bit where you didn't mention @markie at all :p

I am planning on being in Barcelona later this year, it will be great to meet you!

-mike

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