Problem/Motivation

In the Toastify module, we're trying to intercept the status messages in order to render them as toasts. We're doing this by extending the Drupal\Core\Render\Element\StatusMessages class to add the messages to drupalSettings instead of rendering them using ['#theme' => 'status_messages']. The only problem: the parent class uses get_class() in its lazy_builder and pre_render callbacks, requiring us to duplicate a bunch of code instead of being able to just override the renderMessages method.

Proposed resolution

Use static::class instead of get_class().

Issue fork drupal-3346560

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

DieterHolvoet created an issue. See original summary.

DieterHolvoet opened merge request !3604

dieterholvoet’s picture

dieterholvoet commented
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3336570: Compatibility with messages in details (permanent messages)
smustgrave’s picture

smustgrave commented
Category: Task » Feature request
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

This seems like more a feature request.

Either way think test coverage should be included.

dieterholvoet’s picture

dieterholvoet commented
Status: Needs work » Needs review

I wrote a test. I don't have much experience writing tests though, so any feedback is welcome.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems there was a CI failure with the test


  /**
   * {@inheritdoc}
   */
  public static function renderMessages($display = NULL) {
    return ['#plain_text' => 'test successful!'];
  }

}

/**
 * Element info manager that returns a test status messages element.
 */
class TestElementInfoManager extends ElementInfoManager {

  /**
   * {@inheritdoc}
   */
  public function createInstance($plugin_id, array $configuration = []) {
    if ($plugin_id === 'status_messages') {
      return new TestStatusMessages($configuration, $plugin_id, $this->getDefinition($plugin_id));
    }
    return parent::createInstance($plugin_id, $configuration);
  }

}

Not sure you can declare multiple classes within the test, least that I have seen. Could these be private functions for the main class or part of the setup()?

Did run the test locally though without the fix and it did fail which is good means it's a valid test run

dieterholvoet’s picture

dieterholvoet commented
Status: Needs work » Needs review

Fixed the test.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Good job!

Confirmed the test fails locally without the fix so it's valid.

Small change and don't see anything glaring.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

The MR does not merge cleanly.

dieterholvoet’s picture

dieterholvoet commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

I rebased the MR against the latest 10.0.x.

  • catch committed 777df298 on 10.0.x 
    Issue #3346560 by DieterHolvoet, smustgrave: Allow extending...

  • catch committed fdf05ab0 on 11.x 
    Issue #3346560 by DieterHolvoet, smustgrave: Allow extending...
catch’s picture

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

I'm a bit borderline on the test coverage here, also discussed with @longwave who was also somewhat borderline. We have a general rule about using late static binding unless there's a very good reason to, so I don't think this is something that would regress, and then we need to maintain the tests over time. Also thought about whether we could enforce this with phpstan instead of tests although that might be taking things to far.

On balance I went ahead and committed this without the test coverage.

Committed/pushed to 11.x and cherry-picked back through to 10.0.x, thanks!

  • catch committed 1513f1cd on 10.1.x 
    Issue #3346560 by DieterHolvoet, smustgrave: Allow extending...

Status: Fixed » Closed (fixed)

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