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

Provide deprecation on ProxyDepositListener #5550

Merged
merged 2 commits into from Mar 18, 2022
Merged

Conversation

hackartisan
Copy link
Contributor

@hackartisan hackartisan commented Mar 17, 2022

followup to #5529

related documentation: https://dry-rb.org/gems/dry-events/0.2/

DefaultMiddlewareStack is configured to use \
Hyrax::Actors::TransferRequestActor and unregister this listener \
in config/initializers/listeners.rb by adding the line: \n
Hyrax.publisher.unsubscribe(Hyrax::Listeners::ProxyDepositListener.new)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this require that they copy listeners.rb to their /config/initializers? Or will the engine version of this file run first and then the local one of the same name will run second?

Copy link
Contributor

Choose a reason for hiding this comment

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

If having a file of the same name causes an override of Hyrax's listeners.rb initializer, would it work to recommend they create a local file called config/initializers/listeners_ext.rb and put the unsubscribe there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are great questions and I spent some time trying this recommendation.

The engine initializers run first. The existence of an initializer by the same name in an app is fine; both load. However, it seems like you maybe have to unsubscribe the same instance of the listener that you subscribed? For example, if I put an unsubscribe in the local initializer it doesn't affect the listener list. If I put another subscribe of the same listener then it's in there twice (once for each instance).

So the options I see are

  • Don't subscribe the listener in our initializer. If someone has for some reason subscribed it in their own, they will see the deprecation. Probably no one has done this and this means no one will ever see the deprecation.
  • Have a noisy deprecation no one can do anything about. I guess make it a lot shorter.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first option has a great deal of appeal. I wouldn't like have a noise deprecation with no way to quiet it.

Copy link
Contributor

@elrayle elrayle left a comment

Choose a reason for hiding this comment

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

See question about deprecation message.

Copy link
Contributor

@elrayle elrayle left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@eliotjordan eliotjordan merged commit da8f6d3 into main Mar 18, 2022
@eliotjordan eliotjordan deleted the 5457-deprecate-listener branch March 18, 2022 19:43
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.

None yet

3 participants