Problem/Motivation

PHP 8.5 introduced #NoDiscard attribute which is helpful for testing and to indicate that a function's return value is important and should be consumed.

Also core already depends on polyfill-php85

Steps to reproduce

See examples in https://www.php.net/manual/migration85.new-features.php#migration85.new-...

Proposed resolution

Use #[NoDiscard] only in the following situations:

  1. Methods where mistakes in API usage can cause critical bugs. Some examples are security, access control, and encryption methods.
  2. Methods on value objects which return a new object instead of altering it. Some examples are AccessResult and CacheableMetadata.

Document the decision at Coding practices.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 nodiscard.png78.78 KBandypost

Issue fork drupal-3560672

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

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

As polyfill already added core can use the attribute on earlier versions of PHP

https://github.com/symfony/polyfill-php85

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
joachim’s picture

joachim commented

I'm not sure what the polyfill serves here, unless elsewhere in Symfony there's magic which detects the #NoDiscard attribute in static code analysis.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

PHPStan supports #[NoDiscard]: https://github.com/phpstan/phpstan/releases/tag/2.1.28

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

joachim’s picture

joachim commented

What else do we need to do here?

andypost’s picture

andypost
he/him
Primary language Russian
commented

@joachim as summary said we need to mark sensitive functions with this attribute so consumers will throw if result is not used

joachim’s picture

joachim commented

I mean, before we tackle the child issues to actually add the attribute, is there anything else to do here?

andypost’s picture

andypost
he/him
Primary language Russian
commented

My idea was to apply it to few places like AccessResult, CacheableMetadata, cache merge, validators, query builders, render array builders

I will create MR to give a try asap but let's find more places where it helpful

andypost opened merge request !14887

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new nodiscard.png78.78 KB

Somehow PHPStan does not catch it but PHPStorm showing

In core/modules/media/src/MediaAccessControlHandler.php:130 — 

$entity_access->andIf(...)

usage

andypost’s picture

andypost
he/him
Primary language Russian
commented

it works dynamically 

    Media Access (Drupal\Tests\media\Functional\MediaAccess)
     ✘ Media access
       ┐
       ├ Exception: User warning: The return value of method Drupal\Core\Access\AccessResult::andIf() should either be used or intentionally ignored by casting it as (void)
       ├ Drupal\media\MediaAccessControlHandler->checkAccess()() (Line: 130)
andypost’s picture

andypost
he/him
Primary language Russian
commented

PHPStan does not catch because we have checkThisOnly: true and I'm not sure it viable to allow it as much more errors appear

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3425412: Bump PHPStan rule level to 3
andypost’s picture

andypost
he/him
Primary language Russian
commented

So I think it ready to file child issues and it's enough to add the attribute to interface only

andypost’s picture

andypost
he/him
Primary language Russian
commented

Interface only attribute is not working(

andypost closed merge request !14887

andypost’s picture

andypost
he/him
Primary language Russian
commented

on direct class call PHPStan works https://git.drupalcode.org/issue/drupal-3575572/-/jobs/8642189

andypost’s picture

andypost
he/him
Primary language Russian
commented

Filed few child issues

andypost reopened merge request !14887

joachim’s picture

joachim commented

I'm confused - we're filing child issues, but the MR here seems to be all the changes?

andypost’s picture

andypost
he/him
Primary language Russian
commented

I just tested here different approaches, child issues are to work on

dcam’s picture

dcam commented

I looked at these issues last night and had a couple of thoughts:

  • It would be nice for the child issues to include the justification for why the attribute should be applied to the included functions.
  • Since the attribute will cause warnings to be emitted by code that's in violation of it, then shouldn't these issues have change records?
joachim’s picture

joachim commented

> It would be nice for the child issues to include the justification for why the attribute should be applied to the included functions.

Because not using the return of those is probably (or even always!) a mistake. Same reason for all of them really!

dcam’s picture

dcam commented

I figured that out from reading the docs. But that doesn't mean someone can understand at a glance why that's the case for a particular function or set of functions, especially if they're looking at an API they aren't familiar with.

andypost’s picture

andypost
he/him
Primary language Russian
commented

would be great to find some better wording then deadcode

andypost’s picture

andypost
he/him
Primary language Russian
commented

still the most illustrative example is https://git.drupalcode.org/project/drupal/-/merge_requests/14887/diffs?c...

andypost’s picture

andypost
he/him
Primary language Russian
commented

Guideline to reuse result as state got changes after call

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Adopt #NoDiscard attribute » [policy, no patch] Decide if and where to adopt the #[NoDiscard] attribute
Status: Active » Needs review

Coming from #3575572: Add [#NoDiscard] to Utility classes which added it to multiple classes:

\Drupal\C‎omponent\Utility\Color
\Drupal\C‎omponent\Utility\Crypt
\Drupal\C‎omponent\Utility\Html
\Drupal\C‎omponent\Utility\NestedArray
\Drupal\C‎omponent\Utility\Tags

To me I don't see the need to add it to Color or Tags, if we go down that route then we could add it to practically any method that doesn't have a void return type, but I'm not sure that's really necessary.

Initially I think this should be only added to security, access control or encryption related methods, where mistakes in API usage can cause critical bugs. However, I'm open to suggestions as to other types of API as well?

joachim’s picture

joachim commented

> Initially I think this should be only added to security, access control or encryption related methods, where mistakes in API usage can cause critical bugs

+1

Though I would add to that, methods on value objects which return a new object instead of altering it.

So: AccessResult and CacheableMetadata, for instance.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Though I would add to that, methods on value objects which return a new object instead of altering it.

+1. This is one of the first thoughts I have when thinking about #[NoDiscard]. E.g. \Drupal\jsonapi\JsonApiResource\LinkCollection and everywhere there's some kind of return new static (maybe aside of constructors, but maybe we want _also_ constructors).

Edit: I added another "non security" example because of AccessResult and CacheableMetadata could (should) even be considered security, access control related methods.

quietone’s picture

quietone commented
Issue summary: View changes

Tried to put the when to use this into the proposed resolution

smustgrave’s picture

smustgrave commented

Based on the current proposed solution is there consensus those are good guidelines to use this?

smustgrave changed the visibility of the branch 3560672-adopt-nodiscard-attribute to hidden.

smustgrave’s picture

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

The proposed changes makes sense and there's been no follow up since they were added. What would be next?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We need to figure out where to document this decision - either in existing documentation somewhere or a new page.

smustgrave’s picture

smustgrave commented

Maybe a page under https://www.drupal.org/docs/develop/security ?

quietone’s picture

quietone commented
Issue summary: View changes

@smustgrave, that does make sense. But considering we have other Coding Practices I think a more general solution is needed. For a long term solution I think coordination with the coding standards committee is needed.

In the meantime, this issue came up in the committer meeting and catch and I thought the best way to make progress is to make a page in the core policies for this type of topic. The new page is Coding practices.

catch’s picture

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

New page looks great for now, if we find a more permanent place/approach we can move it then. Going to go ahead and mark this 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.

smustgrave’s picture

smustgrave commented

Love the new page and image it has other possibilities!

smustgrave’s picture

smustgrave commented

Could it be an announcement?

Status: Fixed » Closed (fixed)

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