Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event-based STOMP header generation #839

Merged
merged 10 commits into from
Sep 29, 2021

Conversation

adam-vessey
Copy link

@adam-vessey adam-vessey commented Jun 23, 2021

JIRA Ticket: N/A

  • Other Relevant Links (Google Groups discussion, related pull requests, Release pull requests, etc.)

Fixes Islandora/documentation#1856

What does this Pull Request do?

Introduces an event-based mechanism to generate headers for STOMP messages, so other modules can subscribe to the event to influence what headers are set.

What's new?

We dispatch a new StompHeaderEvent (islandora.stomp.header_event) event, which can be subscribed to in other modules in order to add/alter the headers which get used for the STOMP message.

There's a base subscriber for this new event included with a very low priority which maintains the current behaviour of generating and adding the token, but it has been made to avoid overwriting an Authorization token, if there's already one set.

One divergence is the addition of the aud claim; however, the implementation has allowed JWTs without this claim to continue being accepted, expecting that we will remove this ability in the future.

May be a small PR in the future making use of this to address Islandora/documentation#1857

How should this be tested?

Pretty much just regression test, creating some content with derivatives, and ensure that:

  • The derivatives are created
  • Things are indexed as expected
  • Things are deleted from the index accordingly.

Additional Notes:

Any additional information that you think would be helpful when reviewing this PR.

Example:

  • Does this change the interface, add a new feature, or otherwise change behaviours that would require updating documentation? No?
  • Does this change add any new dependencies? No.
  • Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)? No.
  • Could this change impact execution of existing code? Unlikely.

Interested parties

@Islandora/8-x-committers

Copy link
Member

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

This works fine for content nodes, but adding media is failing.

use Drupal\Core\Site\Settings;
use Drupal\Core\StreamWrapper\StreamWrapperManager;
Copy link
Member

Choose a reason for hiding this comment

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

StreamWrapperManager is still used on line 109/53, I tried to add an audio media and got a WSOD.

Stacktrace in the apache log refers to

... Error: Class 'Drupal\\islandora\\Plugin\\Action\\StreamWrapperManager' not found in /home/vagrant/all_claw/islandora/src/Plugin/Action/EmitFileEvent.php ...

Copy link
Author

Choose a reason for hiding this comment

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

Ah... stuff was originally developed against (a fork of) 1.1.1... which was still using the (now deprecated) $fileSystem->uriScheme() thing... patch applied relatively cleanly (IIRC), and the actions were giving a checkmark... I guess this class isn't actually covered under any (unit) tests?

Anyway... the $fileSystem being injected here is no longer being used. Should we drop it? Would rather simplify the class. Removal should be inline with Drupal's policies WRT protected properties?:

Protected properties on a class are always considered @internal unless they're on a base class marked with @api

Copy link
Member

Choose a reason for hiding this comment

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

Probably no test, if you want to add a unit test that would be 🥇 . As for the $fileSystem, you are correct that it does appear to not be necessary, I'd vote to get rid of it and simplify.

Copy link
Author

Choose a reason for hiding this comment

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

Rebased on top of HEAD, and added the reference here back in... left $fileSystem (for now?).

Copy link
Author

Choose a reason for hiding this comment

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

Did take a look at how tests might be written for this ::generateData() stuff; however, the method is protected... would get into either changing the visibility (and pushing such a change up into the interface and propagating around), using reflection to adjust the visibility when running the test (which is relatively simple but gross) or mocking/loading up all the services required to ::execute() the event, which... is... rather more complex.

... start wondering if things should be structured differently, to facilitate testing?... anyway...

@adam-vessey adam-vessey force-pushed the upstream-stomp-headers branch from 81bd3f2 to 1e5a89a Compare September 27, 2021 17:23
Copy link
Member

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

👍

@whikloj whikloj merged commit d800748 into Islandora:8.x-1.x Sep 29, 2021
@adam-vessey adam-vessey deleted the upstream-stomp-headers branch July 18, 2022 14:35
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.

STOMP message headers are not alterable
2 participants