Broken/missing handler after update to Facets 3.0.4: Restore facets_exposed_filters.module file and use LegacyHook for Drupal 10

Good point. I think we should delare a minimum version of D11 instead of re-introducing procedural hooks.
I think that there's simply no time to support both.

As an alternative, I've restored the .module file and added LegacyHook implementations so that the hooks are not fired twice in Drupal 11.

Would be great to get both this and #3605291: facets_preprocess_facets_item_list() is broken in Drupal 11.2 and older fixed. Currently, this breaks tests for other modules. Just increasing the required version now, after the fact, wouldn’t fix them either since then the current version would still be used. In my opinion, we should first fix the code to actually run with the Drupal versions we declare support for and only then increase the requirement and remove BC code. Otherwise, it’s gonna be a bit tricky for modules that want to test their compatibility with Facets.

Drupal 10 will reach EOL in a few months. I don't think that we should go back to procedural hooks for that short period of time.
For sure, we made a mistake, but it is impossible to take care of all compatibility layers to Drupal 10, Drupal < 11.3 and Drupal 12 when facets has no sponsors.

For Search API Solr I just removed the facets related tests for the Drupal 10 branch.

Regarding the current MR, if we consider to merge it, the functions should not copy the code but delegate to the OO Hooks which are available as services.

If 3.0.x only works on Drupal 11, its info file should be updated to say this.

Merge request 397 deals with this issue by declaring Drupal 11.3 as the minimum. This appears to be the real current minimum.

Merge request 397 shows that there is more broken when switching to D11 only. Though that could be facets test configuration only not being compatible with D11.

I think that D10 support should be restored, and then in a follow-up issue work can be done to make this fully compatible with 11 or greater.

Should the 2 other issues be closed as duplicates?

Changing status based on the current proposed resolution of D11 only.

The first thing should probably be an issue to get tests to pass. There needs to be a decision by the maintainer about whether 3.0.x will work on D10 or not. At the moment, it uses things that are only on D11. It would be sensible that 2.0.x is for D10 and D11 while 3.0.x is for only D11.

Why did the huge OO hooks migration tag along with a "'Show only one result' for exposed filters" issue anyway?

Backward-incompatible code changes should be released in major version releases, or at least not in patch version releases. Why should 3.0.3->3.0.4 break Drupal 10 sites?

Also, the argument that it's fine to break Drupal 10 because it's EOL soon anyway is a bit cynical. Developers who are trying to upgrade to Drupal 11 will want to upgrade modules to the latest compatible versions *before* finalizing an upgrade to Drupal 11. Right now the latest version of this module that is allowed to be installed on Drupal 10 breaks it.

The module needs to be updated so that the latest "Drupal 10 compatible" version actually works with it, while a later version is reserved for Drupal 11.

Proposed resolution:

* Release a full revert to 3.0.3 as 3.0.5.
* Re-release 3.0.4 as 3.0.6 but with ^10 removed from composer version compatibility.

I recommend staying on patch versions so that Drupal 11 sites which require ~3.0.0 and which have already upgraded to 3.0.4 do not get stuck on 3.0.5.

I am a big +1 to MR !391 it brought back my D10 facets that were working well. I am hoping to move to D11 but gotta get to D10 from D7 first... it's been a long haul! It should be a shorter trip once we get to D10... but with 300+ contrib modules (lots of which I am now co-maintaining to get across the finish line), basically asking for pity 🥺

Thank you @mradcliffe for the MR!

There seems to be more support here to restore D10 than to lock in D11 support only. @mkalkbrenner hoping to swing your stance.

* Re-release 3.0.4 as 3.0.6 but with ^10 removed from composer version compatibility.

My understanding is that version compatibility is not supposed to be dropped on a patch-level release, so instead of 3.0.6, it would be 3.1.0.

I will second that last comment. My personal opinion is that the breaking change should simply be reverted and held for a following new version probably 4.x rather than 3.1 (that strikes me as a major release sort of thing). I should note that this affected at least one of our projects and we're now locked to a stale version to avoid it.

This breaks updates on existing sites, so upgrading the priority.

I updated the MR so the Drupal 10 procedural hooks delegate to the existing object-oriented hook implementations instead of duplicating their logic. This keeps both implementations consistent and ensures future fixes only need to be made once.

I pushed a follow-up to the latest changes. This builds on the delegation approach from the previous commit, which is the right direction I believe. It matches the backwards compatibility pattern recommended in the OO hooks change record (https://www.drupal.org/node/3442349)

What I changed on top of the previous commit:

• Added wrappers for the two hooks in FacetsExposedFiltersThemeHooks (hook_preprocess_bef_checkboxes and hook_preprocess_bef_radios). These were missed, so the BEF checkbox and radio handling would still be broken on Drupal 10.
• Changed the wrappers to use \Drupal::classResolver() instead of new FacetsExposedFiltersHooks(). Both work today because the class has no constructor, but the class resolver keeps working if the hook classes ever get constructor dependencies.
• Restored the @phpstan-ignore-next-line comments that were added earlier at reviewer request. Phpstan flags the LegacyHook attribute when analyzing against a core version that does not have it yet.

Never knew about Drupal::classResolver but apparently it's been around since 8.6.x

Thanks for updating the approach @ramil g, and the quick iteration @phernand42 and @mradcliffe!

Re-titling based on the MR 391 title so it's a little less ambiguous which I RTBC'd

Also a note the failing PHPUnit test is seemingly unrelated FacetJsonAnon

I understand that maintaining contrib modules is demanding, but breaking all Drupal 10 sites using Facets 3 with the 3.0.4 release, and casually dismissing it is not fair in my opinion. Many users will get hit by this, and left in a mild panic. In my opinion, this change should have immediately been reverted in a fresh release. (3.1.0?)

I tried MR 391 with no luck, but downgrading to Facets 3.0.3 did the trick, so I am adding it in the Issue Summary as a workaround.

The longer this goes unresolved the more it reflects poorly on the companies and individuals that maintain this module. Why was a patch version of this module released with breaking changes?

Thanks for creating #3619826: Document SemVer compliance or lack thereof, I feel bad for the users who will do a casual update on Monday, and get hit by this.

So thank you to everyone who is posting MR's and suggesting solutions, I hope this can get solved before to soon :)

I have updated the Issue Summary to help people find this issue, if they search for any of those strings from the error log or Views UI. I also added in the issue title the words a user is most likely to search for, to help them find this issue faster.

If possible, perhaps someone can update "Proposed resolution" and "Remaining tasks" to make it clearer what the plan is?

Merged, will create a new release. I didn't see this issue till it was mentioned to me in slack by a colleague - slack is probably the best way to reach us when things are urgent like they are here. Communication in the style of #31 is not very helpful.

@borisson_ Thank you a bunch for jumping in to commit these! Could you look at including #3618692: Term weight and UID processors are not offered on Drupal 11.4 and #3617839: Call to a member function getSearchApiQuery() as they contribute to the D10 and test failures on the 3.x branch.

Agreed, as a fellow maintainer I don't appreciate that style of #31.

Apologies for the tone, however it's not like I have an expectation that patch releases will never break sites or that breakage will be fixed immediately. Rather the specific manner of breakage deserves scrutiny; a D10-breaking OO hooks refactor was snuck into an unrelated feature request and was not given a proper commit message. The updated module code was then versioned as a "patch" release despite the large diff and this then-active issue.

And... #3618817: REST tests fail: config entities need ConfigEntityResourceTestBase, and the XML charset changed for the rest test failures.

People make mistakes. Give them the benefit of the doubt, especially when many are volunteering. Following any version scheme is a social contract, and none needs to be followed, it would benefit everyone if was followed… but it’s super loose.

Give them the benefit and help them get on track. Maybe offer to help if you have the bandwidth.

I am sure I’ve made this exact semver mistake in haste to get a release out on a 2-5++ yr old dev branch queue not realizing someone(or even me) was planning a bigger version change all while I just wanted to get a release with some bug fixes long overdue… and there is a good chance I’ll do it again!