Contrib.social

Preparing content for a notification can easily fail

Open on Drupal.org →
Created on 28 October 2024, 2 months ago

Problem/Motivation

We do get alerts that are triggered by a different exception, but regularly lead to a follow-up exception that's caused by the push framework.

The exception we see is something like this:

CRITICAL: Site notification: Site: ... some exception fragment ..., Entity can not be rendered: The "node" entity cannot have a URI as it does not have an ID

The real exception, that we're interested in is ... some exception fragment ..., but we don't get all the required details, as the follow-up exception is this: Entity can not be rendered: The "node" entity cannot have a URI as it does not have an ID

This comes from \Drupal\push_framework\ChannelBase::prepareContent and here is what happens:

Whatever the original exception is, the process goes into this method to prepare the content for the notification. That method receives the $entity which contains the content of the notification. And that entity needs to be rendered in order to provide the real content with replaced tokens, etc.

Now, that entity is not an entity that's stored in the database, it's just an entity that exists in memory but hasn't been saved. And therefore, it doesn't have an ID yet. Now, the problem is, that one of the many steps in the try/catch clause wants to create a URI for that entity which throws an exception because the entity doesn't have an ID.

I don't know what the best way to resolve this should be. Giving that entity an ID just for the sake of it? Finding out what's trying to generate the URI and prevent that from happening? Building some other safeguards to prevent this and maybe similar issues?

This needs some serious thoughts, and I hope we don't have to drop that whole approach in the first place.

🐛 Bug report
Status

Active

Version

2.3

Component

Code

Created by

🇩🇪Germany jurgenhaas Gottmadingen

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @jurgenhaas
  • Comment about 2 months ago →
    🇩🇪Germany jurgenhaas Gottmadingen

    We've further investigated today and found out that we selected the node type "page" with the teaser display mode for rendering the notification content. Unfortunately, that teaser was configured to have a field group with media, title, shortened body and a bit more, and the field group also gets a link so that the whole group can be clicked.

    Of course, that's a stupid setup for a notification rendering and we haven't been clever in setting it up this way.

    What we should do for this issue, though, is to provide a dedicated node display mode for notifications only. That should only contain title and body, and nothing else. Then, the rendering should work without any issue. As such, that won't solve the underlying issue, that links to the node would still fail, but not using such links in the first place is what makes sense, and that way the problem is avoided.

    The new display mode with its basic configuration should become part of the PF package so that it can be used as a quick start. For that, the PF default config should make use of those elements OOTB.

  • Comment about 2 months ago →
    🇩🇪Germany danielspeicher Steisslingen
  • Merge request !14Issue #3484146: Preparing content for a notification can easily fail → (Merged) created by danielspeicher
  • Comment about 2 months ago →
    🇩🇪Germany danielspeicher Steisslingen

    @jurgenhaas Can you please take a look if that is correct? Tests were fine without the link around the content. Do we need an update method? I guess, we do.

  • Comment about 2 months ago →
    🇩🇪Germany jurgenhaas Gottmadingen

    @danielspeicher I've done some clean-up:

    • The display mode contained a lot of fields and dependencies that are not available on most Drupal sites.
    • Also removed the third party config from Layout Builder, as this should not be used for the display.
    • Added the default mode push_framework in prepareContent
    • Declared the dependencies in the info file, as they are required by the new config entities

    There remains a weakness in this approach, that it assumes that the node type page does exist and that it has the body field attached to it. But I think we should be OK with that.

    Ideally, we should test this by enabling the module on a fresh Drupal site and see if it loads everything as expected.

    And then, you're right, we should also have an update hook that imports the 2 new config entities. But I wouldn't update the settings, as that could break some setup on a site that managed that setup themselves with their own settings.

  • Pipeline finished with Skipped
    25 days ago
    #368864
  • Comment 25 days ago →
    System Message
  • Comment 25 days ago →
    🇩🇪Germany danielspeicher Steisslingen
  • Assigned to danielspeicher
  • Status changed to Fixed 11 days ago
  • Comment 11 days ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024