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

Move NormalizerAlterReaction and JsonldTypeAlterReaction to jsonld module #1297

Open
seth-shaw-unlv opened this issue Oct 10, 2019 · 5 comments
Labels
Type: feature request a proposal for a new feature in the software (should be justified by a ‘use case’)

Comments

@seth-shaw-unlv
Copy link
Contributor

In a hypothetical world where someone uses the jsonld module without islandora they may want to leverage the JsonldTypeAlterReaction.

E.g. controlled access terms' corporate body type should use this reaction but currently doesn't because it creates a dependency on the islandora module.

@seth-shaw-unlv
Copy link
Contributor Author

seth-shaw-unlv commented Oct 10, 2019

Clarification: we could add the necessary context config to make corporate_bodies work by placing it in install/optional/ as a stop-gap; but then it would only work as expected when used with islandora. Installing jsonld and controlled_access_terms without islandora would cause it to silently fail.

@seth-shaw-unlv
Copy link
Contributor Author

I looked through what would be necessary to do this again today. Most of it could move over cleanly.

The blocker is NormalizerAlterReaction's use of IslandoraUtils' getRestUrl which we use to get a consistent URL. We could move this helper function to the JSONLD module; but the helper function is used more broadly (i.e. in the EventGenerator and LinkHeaderSubscriber) for more formats than just jsonld, which feels disingenuous.

The upshot: either;

  1. We add the necessary config to controlled_access_term with things as they currently stand; which would enforce a dependency on both jsonld and islandora to get this feature;
  2. We move over the helper-function to the JSON-LD module which expands it's functional scope a little beyond JSON-LD and enforces an Islandora-ism on it;
  3. We don't try to reuse the context and simply rely on controlled_access_terms_jsonld_alter_normalized_array as we have for other bits (yes, it is a re-implementation, but it avoids the other issues); or
  4. (unlikely) Figure out some other way to get consistent URLs that doesn't force us to use a helper.

I think using controlled_access_terms_jsonld_alter_normalized_array is probably the simplest solution that limits dependencies.

@dannylamb
Copy link
Contributor

I find option 2 the least objectionable, FWIW.

@seth-shaw-unlv
Copy link
Contributor Author

Ok, @dannylamb. I'll go with that if nobody objects; but I'll give the other @Islandora/8-x-committers a chance to weigh in before I move forward.

@whikloj
Copy link
Member

whikloj commented Oct 18, 2019

Because the getRestUrl() method relies on a rest endpoint for entities, I think that means we also need to add a dependency on rest to jsonld.

I'd like present another option of having controlled_access_terms implement its own abstract class and a context setup for the json-ld hook, which could be completely separate from Islandora's?

Then things written for controlled_access_terms would be isolated from those written for islandora (and vice-versa) and both of these don't affect people using the json-ld module to just serialize their data (and not muck around in it).

As I did the original work in Islandora and I have the most virulent desire to keep jsonld pure, I'm happy to do the work in controlled_access_terms.

@kstapelfeldt kstapelfeldt added Type: feature request a proposal for a new feature in the software (should be justified by a ‘use case’) and removed feature request labels Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: feature request a proposal for a new feature in the software (should be justified by a ‘use case’)
Projects
Development

No branches or pull requests

4 participants