Open afred opened 6 years ago
@cam156 We'd like to do this in the maintenance sprint next week (week of 9/17). Are there other tickets we should work on to enable the deprecation of Solrizer? README updates, etc? Do you have a sense (or maybe @afred?) about where the best place to put the deprecation warning would be - IE, on gem install vs calling certain Solrizer methods?
I'd say put a deprecation warning on the installation, directly above the module declaration (so it's triggered when you first require 'solrizer'
, and it might not hurt to put one on Solrizer.solr_name
, which I'm guessing is how it is most frequently used.
It was my understanding that using the Solrizer gem was slated to become a deprecated practice. If that's not the case, then I agree, don't issue deprecation warnings. But if it is the case that we want to discourage use of the solrizer gem, then where else should we be issuing messages?
It's deprecated (as in we discourage its use) in modern versions of Hyrax/HydraHead, but I still use it with my very old Hydra-Head (7.x) applications. I'd prefer not to see deprecation warnings there, because it's perfectly reasonable to use it in an old Hydra-head.
Cool. I guess I misunderstood the fate of Solrizer. I assumed we'd soon stop providing support, but I guess that's incorrect?
I'm fine closing this ticket as well as the PR that would close it. But what's then the best way to discourage it's use in Hyrax? For instance, what prompted me to create this ticket in the first place was that our Hyrax app we were using Solrizer.solr_name
in our indexers. If that's discouraged, is there a way we can discourage it programatically, so that programmers who may have missed it in the release notes can be made aware in code?
@afred I think we do stop providing support as in "we're not going to continue to develop this further", I just don't want to degrade the existing experience.
I understand not wanting to throw a bunch of deprecation warnings if the impelmenters intent is to keep using Solrizer gem.
But what happened in our case was that we were using Solrizer.solr_name
in our indexers, and then after an upgrade suddenly started getting uninitialized constant Solrizer
after an upgrade, because it was no longer part of the bundle.
This is the type of error I think we should be responsible for providing information on what to do.
The quick fix, in this case, is to just explicitly add solrizer
to your bundle. But that's actually not the recommended practice.
So I'm just trying to figure out what is the best way to encourage best practice, and avoid head scratching errors on upgrades. The answer very well could be simply to make sure to put something in the release notes. But it's always nicer, imo, to get hints from things like deprecation messages, if possible.
@afred you would never get that unless you have a dependency on solrizer and you are not explicitly including it in your bundle. The same thing could happen if you depend on some other library that is bundled by another dependency (say Faraday or Nokogiri). You should just adjust your Gemfile so that you list out all of your dependencies that you use explicitly.
It's slightly different though, because both Solrizer and ActiveFedora are both managed by the Samvera community. At the same time, it was recommended practice to use Solrizer.solr_name
in your indexers (or at least there were lots of examples showing that's how to do it), and then we changed the recommendation to not do that.
I'm just looking for the best solution that programmers can do to help implementers get on the same page about what the best practice is.
And again, maybe that solution is to just really emphasize the release notes.
There's nothing stopping you from using Solrizer if you want to. If you use a library you need to declare it in your Gemfile. So, this seems like it could be fixed by adding an upgrade note like:
Solrizer is no longer a dependency of Hyrax. Add
gem 'solrizer'
to your gemfile if you want to continue to use solrizer
FYI - I've proposed deprecation of the gem entirely. I kind of agree that older installations still requiring it probably don't benefit from code-level deprecation warnings.
See https://github.com/samvera/solrizer/pull/55 And https://groups.google.com/forum/#!topic/samvera-tech/T83tGB2Sypo
Background: Solrizer has been absorbed by
ActiveFedora
. You should useActiveFedora.index_field_mapper
where you used to useSolrizer
.Done when: deprecation warnings are issued when attempting to use
Solrizer
.