Problem/Motivation

Drupal currently has 3 message types:

  • status
  • warning
  • error

These are found in MessengerInterface as constants. This interface also defines methods to directly add a message for each of these types, such as addWarning.
Any string "type" could be passed as the second argument to addMessage however there are other places in core where these 3 and only these 3 types are allowed.

For example, the render array output by StatusMessages passes an array with these 3 types to themes,

'#status_headings' => [
  'status' => t('Status message'),
  'error' => t('Error message'),
  'warning' => t('Warning message'),
],

If a new type is used then the heading text won't display properly in themes like classy, where you find: {{ status_headings[type] }} in status-messages.html.twig

If these types were not hardcoded in Drupal core, or if there was a way to extend these 3, then new message types could be added for unique events.

Proposed resolution

Determine a way to extend the 3 message types.
Add additional info such as status headings and fallback classes. New types should be able to "fallback" to existing styles. For example, a "success" type could add messages--status class to use the existing "status" styling.

Remaining tasks

  1. Add option to add additional Messenger types from a module
  2. Make these new types discoverable for JavaScript Drupal.Message see: getMessageTypeLabels

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 interdiff_3-5.txt2.65 KBzrpnr
#5 3100473-5.patch8.1 KBzrpnr
#4 3100473-3.patch7.91 KBgabesullice
#4 interdiff.txt2.34 KBgabesullice
#2 3100473-2.patch7.88 KBzrpnr

Comments

zrpnr created an issue. See original summary.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
commented
Status: Active » Needs review
StatusFileSize
new 3100473-2.patch7.88 KB

I discussed this with @gabesullice and we pair programmed a bit to get a prototype working. The bulk of the credit goes to him for the idea to use YamlDiscovery to parse the "types". I added onto his code with a base_types property so that new types can have a fallback class.

This patch adds a StatusMessageTypeHandler service which discovers types in [module-name].status_message_types.yml
The 3 main types are in system.status_message_types.yml while any module can add additional types.

For example, in mymodule.status_message_types.yml

info:
  heading: 'Info message'
  base_types: ['status']

This provides a type "info" which would have styling like an existing status message.

Setting to "Needs Review" to check if I missed any tests, and I'd appreciate any feedback about this approach.
This also still needs some thought around the js Message API.

Status: Needs review » Needs work

The last submitted patch, 2: 3100473-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new interdiff.txt2.34 KB
new 3100473-3.patch7.91 KB

Paired a bit more with @zrpnr on this. He mentioned that he was not sure how a module could override or extend a type provided by another module. In the end, we added the concept of "priority" so that modules can override other statuses. That's in this interdiff.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
commented
Status: Needs work » Needs review
StatusFileSize
new 3100473-5.patch8.1 KB
new interdiff_3-5.txt2.65 KB

I wasn't clear if there is an order or "weight" to the loaded yml files, having a specific priority key makes sense to me to allow a module to override an existing type.

+++ b/core/lib/Drupal/Core/Messenger/StatusMessageTypeHandler.php
@@ -0,0 +1,64 @@
+            $this->types[$machine_name] = $type + self::$messageTypeDefaults;

This is a much more elegant way to set the defaults than I had before!

+++ b/core/lib/Drupal/Core/Messenger/StatusMessageTypeHandler.php
@@ -0,0 +1,64 @@
+  public function getStatusMessageHeadings() {
+    return array_column($this->getStatusMessageTypes(), 'heading');
+  }
+
+  public function getStatusMessageBaseTypes() {
+    return array_column($this->getStatusMessageTypes(), 'base_types');
+  }

Using array_column removes the key which is needed to select the value in the template.

+++ b/core/themes/claro/templates/misc/status-messages.html.twig
@@ -24,12 +25,13 @@
+  {% set base_classes = base_types[type]|map(t => "messages--#{t}") %}

Most of the tests were failing because of not checking for base_types[type], and the BigPipe test failed because the extra variables added spacing to the template and it uses a strict comparison.

Status: Needs review » Needs work

The last submitted patch, 5: 3100473-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/lib/Drupal/Core/Messenger/StatusMessageTypeHandler.php
@@ -0,0 +1,69 @@
+          elseif (floor($type['priority']) > floor($this->types[$machine_name]['priority'])) {
+            $this->types[$machine_name] = $type + self::$messageTypeDefaults;
+          }

It's not clear what this is doing here. Is the idea that if two modules provide the same name, then one with the higher priority will replace the existing one?
That's a departure from how Drupal usually accomplishes replacements (via alters).

Afaik this would be the 3rd usage of \Drupal\Core\Discovery\YamlDiscovery, the others being routes and permissions.
The rest of YAML discovery is done via plugins. Layouts, breakpoint, config translation, help topics, and the 4 types of links (actions, tasks, menu, contextual).
If using plugins, a lot more typical Drupalisms are easy to introduce. Alter hooks, sorting, more standardized handling of default values, translatability of properties, the option for dynamically defined message types, etc.

Finally, aside from plugins vs straight YAML: what about themes? Would it make sense for a theme to provide its own message type?

gabesullice’s picture

gabesullice
Primary language English
commented

@tim.plunkett, thanks for the review! This is mostly in @zrpnr's wheelhouse, but I can speak to some of the design decisions.

Is the idea that if two modules provide the same name, then one with the higher priority will replace the existing one?

Precisely.

That's a departure from how Drupal usually accomplishes replacements (via alters).

I see your point, but I'm not convinced that an alter hook makes sense. I think the API as-written is both simple and sufficient. Something Drupal needs to be better about as a whole :)

Afaik this would be the 3rd usage of \Drupal\Core\Discovery\YamlDiscovery, the others being routes and permissions.

I think you're correct about usages.

The rest of YAML discovery is done via plugins... If using plugins, a lot more typical Drupalisms are easy to introduce. Alter hooks, sorting, more standardized handling of default values, translatability of properties, the option for dynamically defined message types, etc.

Using the basic YAML discovery instead of introducing a new plugin system was done very intentionally. I'd argue that status message types are even less complex then permissions.

None of the plugin features that you describe are needed for the known use case. Turning this into a plugin system is probably premature abstraction. If those features ever become necessary, it would not be difficult to "upgrade" the proposed design to a full-blown plugin implementation in a BC compatible way (e.g. make a plugin deriver that can read the legacy yaml files).

Finally, aside from plugins vs straight YAML: what about themes? Would it make sense for a theme to provide its own message type?

I'll defer to w/e @zrpnr's opinion of this is. However, my gut reaction is that the answer is "no". A theme merely styles message types. It would be a pretty rare instance for A) a theme to set a message and B) the core message types to be insufficient for that message.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

digitaldonkey’s picture

digitaldonkey commented

> Would it make sense for a theme to provide its own message type?

I think it would make a lot of sense. Why we don't allow extending messages on a JS base?

Making getMessageTypeLabels() a bit less static (like providing addMessageType() or Drupal.Message.customTypes(Array) would be helpful.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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.