Problem/Motivation

\Drupal\comment\Entity\Comment::getSubject() can cause deprecations in PHP 8.1 because it can return NULL.

Steps to reproduce

Run \Drupal\Tests\comment\Kernel\CommentOrphanTest on PHP 8.1

Proposed resolution

Fix \Drupal\comment\Entity\Comment::getSubject() to not return NULL or come up with a better fix.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 interdiff.txt430 bytesandypost
#6 3240171-6.patch457 bytesandypost
#2 3240171-2.patch553 bytesalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3240171-2.patch553 bytes

Here's the fix from the meta.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I think this is fine, we should remove the todo

andypost’s picture

andypost
he/him
Primary language Russian
commented

It removes 17 nitices

  17x: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated
    17x in CommentOrphanTest::testOrphan from Drupal\Tests\comment\Kernel

@larowlan does it need CR?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
/**
   * Returns the subject of the comment.
   *
   * @return string
   *   The subject of the comment.
   */
  public function getSubject();

I don't think we need a CR, we're just tightening it up to satisfy the interface

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3240172: \Drupal\comment\Entity\Comment::getThread() can cause deprecations on PHP 8.1
StatusFileSize
new 3240171-6.patch457 bytes
new interdiff.txt430 bytes

Patch without todo, the same could be done in related

daffie’s picture

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

Looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think this is the best fix here. getSubject() ends up in tonnes of places that expects strings... in tokens and in places like CommentForm which does:

    // Validate the comment's subject. If not specified, extract from comment
    // body.
    if (trim($comment->getSubject()) == '') {

One thing that is kinda interesting is that ::getSubject() and ::label() (which is the subject for a comment) will now return different values for a NULL value.

  • catch committed f496293 on 9.3.x 
    Issue #3240171 by andypost, alexpott, larowlan: \Drupal\comment\Entity\...
catch’s picture

catch
he/him
Primary language English
commented

Comment labels are generally very odd - holdover from pre-field API implementations in Drupal 6 and earlier. We should probably have an issue open to revisit, but not opening because I have a feeling there's already one around somewhere.

catch’s picture

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

Committed/pushed to 9.3.x (forgot to mention that).

Status: Fixed » Closed (fixed)

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