Skip to content

Conversation

@bserem
Copy link
Contributor

@bserem bserem commented Apr 29, 2025

What does this change?

  • Call Out Box paragraph, found under localgov_subsites_paragraphs uses a media field named localgov_background_image.
  • Data from that field is then loaded in
    $fid = $paragraph->get('localgov_background_image')->entity->field_media_image[0]->getValue()['target_id'];
  • If a media file that is used in Call Out Box gets deleted then the site results in a WSOD and the user can't view or edit the node in order to fix it.

Checking if the field is empty:

if (!$paragraph->get('localgov_background_image')->isEmpty()) {

is not correct in this case, as we should be checking if the entity actually exists

How to test

  1. Add a node with a call out box with a media
  2. See the node
  3. Delete the media from the media library
  4. See the node -> you get a PHP error

How can we measure success?

Deleting a media doesn't break the page

$variables['button_url'] = Url::fromUri($button['uri']);
}
if (!$paragraph->get('localgov_background_image')->isEmpty()) {
if ($paragraph->get('localgov_background_image')->entity) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the first time this section has caused us pain.
#125

Feels like it's brittle and needs tests. There is however nothing and this will fix the specific issue.

@stephen-cox stephen-cox self-requested a review May 23, 2025 13:37
Copy link
Member

@stephen-cox stephen-cox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the issue, but we're getting the entity to check if it exists and then again on the next line to get the file ID.

I suggest assigning the entity to a variable when checking for it doesn't need to be fetched for a second time.

E.g.

    if ($file = $paragraph->get('localgov_background_image')->entity) {
      $fid = $file->entity->field_media_image[0]->getValue()['target_id'];

Agree with @ekes that tests for all of this are needed, but we can create another issue for that.

@stephen-cox
Copy link
Member

@bserem Happy to approve this if you're able to make the requested code change 🙂

@finnlewis
Copy link
Member

@ekes and @stephen-cox still keen to make the change before merging.

ekes added a commit that referenced this pull request Jun 3, 2025
@ekes ekes mentioned this pull request Jun 3, 2025
finnlewis pushed a commit that referenced this pull request Jun 3, 2025
* Fix WSOD if referenced media gets deleted

* Code improvement #246 (review)

---------

Co-authored-by: Bill Seremetis <bill@seremetis.net>
@ekes ekes closed this Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants