Fix for documentation 2069 #931

lucasvanschaik · 2023-02-17T16:53:34Z

GitHub Issue: N/A

Other Relevant Links
https://github.com/orgs/Islandora/projects/7/views/1?filterQuery=event&pane=issue&itemId=3636970
https://git.drupalcode.org/project/context/-/commit/b7e8b7f2295d661dc83513e282df4234c9c390c1
https://git.drupalcode.org/project/context/-/commit/d31cd87b909737abd1beab10e68c62ec29acce35

What does this Pull Request do?

When inserting or updating nodes and/or media, in some cases an event is triggered with the wrong type of entity. This results in error messages such as: "Error generating event: Bundle islandora_object does not exist".

This PR implements two fixes for issues that are already solved in Drupal but not in Drupal\islandora\IslandoraContextManager that extends Drupal\context\ContextManager.

Also it checks if an action is appropriate for the given entity before executing it.

What's new?

Solved: Remaining conditions not being evaluated when failing to apply a context to a previous condition
Abort if the application of contexts has been unsuccessful
Check if an action is appropriate for the given entity
Does this change add any new dependencies? No
Does this change require any other modifications to be made to the repository
(i.e. Regeneration activity, etc.)? No
Could this change impact execution of existing code? Yes, because less actions/events are fired the indexing/derivation process will become quicker. But more rigorous test should be done to make sure that all required actions/events are fired.

How should this be tested?

I got this error when migrating a compound object all at once. We use custom migration steps to migrate from Islandora 7 to Islandora 8. Sharing the yaml files for this migration will not suffice because we use some custom plugins.
Antbrown has some steps to reproduce (that I did not use):

Implement hook_media_update()
Fetch parent node $islandora = \Drupal::service('islandora.utils'); $parent = $islandora->getParentNode($media);
Make changes to parent node and save: $parent->save();
Observe the error: "Error generating event: Bundle islandora_object does not exist" (or whatever bundle your parent node is).

It is very important to test if all index and derivation actions/events that are relevant still get fired.
So test by doing:

a migration of some objects with media
adding some objects with media by hand
check if all derivatives are made and if all is indexed correctly

Documentation Status

Does this change existing behaviour that's currently documented? no
Does this change require new pages or sections of documentation? no
Who does this need to be documented for? developers, but is done in code in PR
Associated documentation pull request(s): none

Interested parties

@Islandora/committers @seth-shaw-asu

seth-shaw-asu · 2023-02-17T18:01:14Z

@lucasvanschaik , the code seems reasonable, but the functional tests are failing. Would you mind running the tests locally to see if you can identify the issue?

…entity type

seth-shaw-asu

Everything here looks fine to me. I was able to generate the error with migrate:rollback and the errors cleared when the PR was applied with all the microservices appearing to work as expected. Creating a new item using the web UI also triggered the appropriate reactions as well.

seth-shaw-asu · 2023-02-21T19:01:09Z

Hey, @Islandora/committers, this PR looks good to me, but I want y'all to have a chance to glance at it before I merge, since y'all weren't tagged initially.

src/IslandoraContextManager.php

jordandukart · 2023-02-24T15:42:13Z

src/PresetReaction/PresetReaction.php

+      // Make sure that the action is appropriate:
+      // either system action or with same type as the entity type.
+      if ($action->getType() === 'system' || $entity->getEntityTypeId() === $action->getType()) {
+        $action->execute([$entity]);
+      }


Is there a particular reason to need to limit to these types here? For example, we at dgi have some custom reactions that are more generic but don't utilize a deriver to specify types directly that no longer get triggered due to the introduction of this short circuiting while testing this.

There are also some contrib module examples that do not do this as well such as views_bulk_edit and pathauto.

Checking the action type against the entity type prevents the action (meant for node (Islandora object)) to execute on a media.
Does including empty($action->getType()) || $action->getType() === 'entity' in the if statement solve the issues?
Or is this too crude and should the action itself determine if the action with the given entity can be performed? For example by using the access method of the Action class?
At the moment my understanding of derivers is limited; the code above is meant to make sure that an action is not called with the "wrong" entity type. If there is another way, I'm open to suggestions.

I mean I think the current onus is on the configured conditions to limit when an action is executed.

It doesn't look like access is leveraged by any of the existing Islandora actions as of current. Given how the majority of the Islandora based actions just pop things on a queue to be processed by microservices at a system level most access restrictions provided on an individual user/entity won't be relevant.

In normal Drupal land, limiting at the form configuration level would seem like the place to do it and would align with how core tends to operate. That is checking for access when retrieving actions whether it be on the entity query that gets executed or on loadMultiple and filtered after the fact such as https://git.drupalcode.org/project/drupal/-/blob/9.5.3/core/modules/views/src/Plugin/views/field/BulkForm.php#L120-131 or https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/src/Plugin/views/field/SearchApiBulkForm.php#L42-49. This form level filtering approach is not feasible imo given how these context reactions are configured to be setup without any entity context until runtime.

I think what you are proposing makes sense in that likely the type should be checked and used as a filtering factor here given the non-standard usage of Context and Actions within Islandora. If this change was to be introduced it'd likely need to be heavily documented and potentially tested as it would be breaking (whether it's introduced in a minor/major release).

Probably would want other @Islandora/committers to chime in here or this may be an issue that should be added to the agenda for an upcoming tech call.

Maybe a simpler solution is better; I understand you reasoning, so why not solve the problem at the location where it appears:
https://github.com/Islandora/islandora/blob/2.x/src/MediaSource/MediaSourceService.php#L99-L101
Here the exception is thrown. But why an exception, why not just return NULL? If the source_field in the source_configuration is not found, then NULL is returned, so why not if the bundle is not found? Returning NULL does not have any (other) negative effects (for so far I can see).

Isn't clear to me how targeting it there helps in this scenario? That is the conditions have already evaluated that an action should be fired and given the forced typification by the MediaSourceService it should be a media entity I think?

I know you mentioned it's tough to really have reproducible steps but perhaps we could connect in Slack to maybe identify the root of this? I have ran into a scenario before based upon underlying assumptions of context in general that it's meant to be operating on a single page.

That is, take a scenario where you may have multiple contexts/actions triggering within a single thread with different entities being targeted. By its nature Context doesn't account for this scenario because in page rendering land it should never be encountered. To facilitate this we implemented our own ContextRepository and the actions that were exhibiting this behavior we moved over to using it (https://github.com/discoverygarden/dgi_actions/blob/1.x/src/ResettableContextRepository.php for reference). As we've unravelled in this thread it's a very hard thing to pinpoint down and reproduce in a more generic sense but perhaps this becomes a catalyst for introducing tests and a potential fix in the event something similar is at play here.

I was able to reproduce the error messages on a starter site box (using the playbook).

I cloned https://github.com/seth-shaw-asu/migrate_apollo (technically, the precursor, but then I made a bunch of updates which have now been committed back to it)

I did the module's requisite copy of the data directory and enabling the module. (See the README.)

I then ran the migrations. That all worked fine, but...

I ran a migrate rollback which then triggered the following errors:

vagrant@islandora8:/var/www/html/drupal$ drush --uri=https://localhost:8000 --userid=1 mr islandora8_metadata,islandora8_media,islandora8_file [notice] Now acting as user ID 1 6/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% [notice] Rolled back 6 items - done with 'islandora8_media' 6/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% [notice] Rolled back 6 items - done with 'islandora8_file' [error] Error generating event: Bundle islandora_object does not exist 1/6 [▓▓▓▓░░░░░░░░░░░░░░░░░░░░░░░░] 16% [error] Error generating event: Bundle islandora_object does not exist 2/6 [▓▓▓▓▓▓▓▓▓░░░░░░░░░░░░░░░░░░░] 33% [error] Error generating event: Bundle islandora_object does not exist 3/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░░░░░░░░░░░] 50% [error] Error generating event: Bundle islandora_object does not exist 4/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░░░░░░░] 66% [error] Error generating event: Bundle islandora_object does not exist 5/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░░] 83% [error] Error generating event: Bundle islandora_object does not exist 6/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% [notice] Rolled back 6 items - done with 'islandora8_metadata' [notice] Switching back from user 1. [error] Message: Error generating event: Bundle islandora_object does not exist

Applying the PR caused those "Error generating event" messages to not appear.

Thanks @seth-shaw-asu will look again!

So, figured this out.

It's is indeed the same issue we saw at dgi in terms of contexts not being reset between evaluations. That is, previously evaluated contexts within the same thread are retained when additional conditions are evaluated.

@lucasvanschaik at this point can you remove the action filtering so the Context fixes can be merged and I'll open a new issue / PR for the follow-up once I can figure out how best to package all this up?

Contained in #932.

jordandukart · 2023-02-24T16:56:01Z

Review comments aside thanks for submitting this @lucasvanschaik as it's a very great catch to pull in those bug fixes to the parent ContextManager class upstream!

jordandukart · 2023-03-13T12:37:59Z

Ping @lucasvanschaik.

Can you remove the action filtering so the upstream Context fixes can be merged?

I diagnosed and fixed the underlying issue with Islandora's implementation of Context in #932 which is currently being reviewed.

This reverts commit aba5052.

…th same entity type" This reverts commit a409d40.

This reverts commit 87f475d.

lucasvanschaik · 2023-03-14T10:31:42Z

I see #932 is already approved. I've reverted the action filtering, so it should be alright now.

jordandukart · 2023-03-14T11:50:38Z

Thank so much for the patience and unravelling enough thread @lucasvanschaik to help diagnose and fix this super old issue!

lucasvanschaik added 2 commits February 17, 2023 16:56

Implement solution for drupal issues 3089660 and 3045666

74755f8

Check if action is appropriate for entity before executing

87f475d

lucasvanschaik mentioned this pull request Feb 17, 2023

Bug? hook_media_update(), $parent->save(), generates EmitMediaEvent for a Node Islandora/documentation#2069

Closed

lucasvanschaik added 3 commits February 20, 2023 21:25

Check if conditions exist before applying contexts to them

4250109

Make sure that the action is appropriate: either system or with same …

a409d40

…entity type

Comment too long

aba5052

seth-shaw-asu approved these changes Feb 21, 2023

View reviewed changes

jordandukart self-requested a review February 23, 2023 19:40

jordandukart self-assigned this Feb 23, 2023

jordandukart requested changes Feb 24, 2023

View reviewed changes

jordandukart removed their assignment Feb 24, 2023

Be consistent with context module, issue 3177007

b89da47

This was referenced Mar 7, 2023

[BUG] Islandora's ContextManager implementation retains references to previously evaluated contexts Islandora/documentation#2213

Closed

Reset contexts before evaluation. #932

Merged

lucasvanschaik added 3 commits March 14, 2023 11:16

Revert "Comment too long"

ee425d2

This reverts commit aba5052.

Revert "Make sure that the action is appropriate: either system or wi…

233a65d

…th same entity type" This reverts commit a409d40.

Revert "Check if action is appropriate for entity before executing"

d041ec3

This reverts commit 87f475d.

jordandukart approved these changes Mar 14, 2023

View reviewed changes

jordandukart merged commit e366da3 into Islandora:2.x Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for documentation 2069 #931

Fix for documentation 2069 #931

lucasvanschaik commented Feb 17, 2023

seth-shaw-asu commented Feb 17, 2023

seth-shaw-asu left a comment

seth-shaw-asu commented Feb 21, 2023

jordandukart Feb 24, 2023 •

edited

Loading

lucasvanschaik Feb 27, 2023

jordandukart Feb 27, 2023 •

edited

Loading

lucasvanschaik Mar 1, 2023

jordandukart Mar 1, 2023 •

edited

Loading

seth-shaw-asu Mar 1, 2023

jordandukart Mar 2, 2023

jordandukart Mar 6, 2023 •

edited

Loading

jordandukart Mar 7, 2023

jordandukart Mar 7, 2023

jordandukart commented Feb 24, 2023

jordandukart commented Mar 13, 2023

lucasvanschaik commented Mar 14, 2023

jordandukart commented Mar 14, 2023

	/**
	* Implements hook_taxonomy_term_delete().
	*/
	function islandora_taxonomy_term_delete(TermInterface $term) {
	$utils = \Drupal::service('islandora.utils');

	// Execute delete reactions.
	$utils->executeTermReactions('\Drupal\islandora\Plugin\ContextReaction\DeleteReaction', $term);
	}

	/**
	* Executes context reactions for a File.
	*
	* @param string $reaction_type
	* Reaction type.
	* @param \Drupal\taxonomy\TermInterface $term
	* Term to evaluate contexts and pass to reaction.
	*/
	public function executeTermReactions($reaction_type, TermInterface $term) {
	$provider = new TermContextProvider($term);
	$provided = $provider->getRuntimeContexts([]);
	$this->contextManager->evaluateContexts($provided);

	// Fire off index reactions.
	foreach ($this->contextManager->getActiveReactions($reaction_type) as $reaction) {
	$reaction->execute($term);
	}
	}

	/**
	* {@inheritdoc}
	*/
	protected function generateData(EntityInterface $entity) {
	$data = parent::generateData($entity);
	$data['source_field'] = $this->mediaSource->getSourceFieldName($entity->bundle());
	return $data;
	}

Fix for documentation 2069 #931

Fix for documentation 2069 #931

Conversation

lucasvanschaik commented Feb 17, 2023

What does this Pull Request do?

What's new?

How should this be tested?

Documentation Status

Interested parties

seth-shaw-asu commented Feb 17, 2023

seth-shaw-asu left a comment

Choose a reason for hiding this comment

seth-shaw-asu commented Feb 21, 2023

jordandukart Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

lucasvanschaik Feb 27, 2023

Choose a reason for hiding this comment

jordandukart Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

lucasvanschaik Mar 1, 2023

Choose a reason for hiding this comment

jordandukart Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

seth-shaw-asu Mar 1, 2023

Choose a reason for hiding this comment

jordandukart Mar 2, 2023

Choose a reason for hiding this comment

jordandukart Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

jordandukart Mar 7, 2023

Choose a reason for hiding this comment

jordandukart Mar 7, 2023

Choose a reason for hiding this comment

jordandukart commented Feb 24, 2023

jordandukart commented Mar 13, 2023

lucasvanschaik commented Mar 14, 2023

jordandukart commented Mar 14, 2023

jordandukart Feb 24, 2023 •

edited

Loading

jordandukart Feb 27, 2023 •

edited

Loading

jordandukart Mar 1, 2023 •

edited

Loading

jordandukart Mar 6, 2023 •

edited

Loading