-
Notifications
You must be signed in to change notification settings - Fork 119
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
Event-based STOMP header generation #839
Conversation
There was a problem hiding this 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; |
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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...
... ensure we're dealing with our tokens.
... _could_ roll more conditionally, with some state set during an update hook; however, seems like unnecessary complexity.
81bd3f2
to
1e5a89a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
JIRA Ticket: N/A
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:
Additional Notes:
Any additional information that you think would be helpful when reviewing this PR.
Example:
Interested parties
@Islandora/8-x-committers