Content model documentation: on edit, fatal errors leading to white screen

[swirtSJW]

Describe the defect

On trying to edit or create a new content model document, I receive a white screen error

Looking at the recent log entries I see

TypeError: method_exists(): Argument #1 ($object_or_class) must be of type object|string, null given in method_exists() (line 113 of /var/www/cms/docroot/modules/custom/va_gov_media/src/EventSubscriber/MediaEventSubscriber.php). [dd.trace_id=9223372036854775807 dd.span_id=721623755462565909]

The line of code was merged Jan 10 2024 as part of this alt text validation PR

The problem is the line of code
if (method_exists($fieldDefinition, 'id'))
assumes that $fieldDefinition is an object when sometimes it can be null.

To Reproduce

Steps to reproduce the behavior:

1. Login as an admin
2. Go to https://prod.cms.va.gov/admin/structure/cm_document/5/edit
3. or https://prod.cms.va.gov/admin/structure/cm_document/add
4. See error

AC / Expected behavior

We should be able to add or edit things without fatal errors.

Additional context

This is surfacing on Content Model Document entities but there is nothing special about them related to image fields. They are just another fieldable entity like nodes, terms, and paragraphs so it is possible this error is happening other places too.

ACs

• Content model documentation can be edited / saved successfully

[swirtSJW]

Could be fixed by changing
if (method_exists($fieldDefinition, 'id')) {

to

if (is_object($fieldDefinition) && method_exists($fieldDefinition, 'id')) {

[swirtSJW]

Looking into this deeper. The fix I proposed above would fix it without a problem and is worth adding the check to prevent the assumption. However the real problem is higher up where the code makes assumptions and even declarations to itself that are not true.

The code is assuming and declaring that $form_object is a \Drupal\media\MediaForm which is not always the case.
It is also assuming and declaring that $entity \Drupal\media\MediaInterface which is not always the case.

[swirtSJW]

Trying to get the definition of the field fails, because the call to
$fieldDefinition = $entity->getFieldDefinition('image'); is giving it the wrong information. The method getFieldDefinition is expecting the name of the field, not the type of the field. image is a type, not the field name.

Here is the difference between giving it the field type and the field name.

Suggest grabbing the field name dynamically rather than hardcoding.
$entity->getFieldDefinition($element['#field_name']);

The field name of "image" only exists on the "Media" entity. All other image fields would have some other name that would never be "image" because they follow entity field names that are field instance dependent and prefixed with "field_".

[swirtSJW]

Why does this fail on content model documents and no where else? Because content model documents are the only entity that has an image field. All other places in the site where images are use an entity reference to media, and media is the only content type where this new code works. Adding an image field to any content type directly, will also throw a fatal error.

Example: Say we wanted to have an image field on events for a map, but we did not want to have the map be reuseable and end up in the media library. I added an image field_map to events and here is the result when trying to create an event

[swirtSJW]

Summary big picture issue: The current code in imageFieldWidgetProcess() is hunting for an image field on every single entity that that Drupal loads. However, inside that function, the assumption is made that it is only ever operating on a "media" entity and this assumption runs into trouble.

There are two possible solutions depending on the intention of code.

1. If the intent of the code is to alter all image type fields everywhere on all entities (node, term, paragraph, media, menu, and content model doc) to add the alt modifications then the internal code needs to not assume it is operating on a media entity. It needs to be more generic. I believe this is the intention, so this is the approach I took with this PR VACMS-16827 Fix loading of alt text field definitions. #16832
2. If the intent of the code is to only alter the image field on media entities and no-where else, then the code needs to be properly scoped to not hunt for image fields on anything other than media entities.

[jilladams]

Noting: checked in with Marcia and Erika on this re: whether Facilities should go ahead and put up a PR for the code change Steve identified, or would like to own the PR. Marcia ok'ed putting up a PR. I've moved the associated PR from draft, and tagged in code reviewers. Not sure if this'll move today as I know a couple of people are out on wellness / before the 3-day weekend, so if not, it'll go after the break.

[jilladams]

(Updating assignment / sprint to reflect PR owner.)

[swirtSJW]

PR was just approved by CMS team and I merged it. This could be closed.