This has been merged to HEAD and will be part of the next release as soon as it is confirmed to work correctly.
I think I finally figured out that failing test: Turns out the admin user just didn’t have the permission to view test entities, that’s why the Views result was empty.
Then why did that pass with the search_api_none cache plugin? No friggin’ clue!
Anyways, still needs a regression test, but now with all those unrelated bugs discovered and fixed I’m extra-motivated to get this over the finish line! I’ll hopefully have some time during the holidays.
Hm, sorry, I still cannot reproduce this.
If you export the view, what’s the value of display.page_1.display_options.fields.search_api_excerpt.plugin_id? It should be search_api_text, if it’s something else then that would explain the problem. I’m just not sure how to fix it if re-saving the view doesn’t do it.
I’ve now created
release 1.9.1 →
which contains these fixes and should be used by everyone who doesn’t want their settings automatically migrated to Key-based storage. (As discussed above, there will probably be some UI assistance for doing this manually in the future, and the searchstax.migrate_to_keys service can still be used by any developers. Alternatively, you can also update to 1.9.0 first to get the automatic migration and only then update to newer versions.)
One further UI adjustment in the MR: There was leftover code in the forms which made the key_id setting default to one of the automatically installed keys when it wasn’t set yet. This should now also be fixed.
Since the pipelines were green, I already merged the MR. Please do keep testing and providing feedback, hopefully we can soon get this to a place where it is as useful and convenient as possible for all users.
I opened a new MR with a small UI adjustment: The descriptions for using keys weren’t very consistent between the settings page and the connector plugin form. I adjusted the former to fit with the latter.
The MR has now been merged, you can therefore also use the dev version of this module now to test the changes and provide feedback. I will create a new 1.9.1 release containing this fix once I have confirmation that the new code works and resolves the problem for users that have the Key module installed but don’t want their SearchStax credentials automatically migrated.
I’m also still leaving this open to discuss further UI improvements we can make here. The process of manually moving the credentials to the respective keys is a bit cumbersome now, especially if there are multiple SearchStax servers on a site.
Is there demand for some UI on the site to automate this process (like it was done before, but now only after explicitly triggering it), and if so, how should that look? Should it be placed on the respective forms or do we want a new, separate page just for this migration, like we currently have for the version checks?
Any input and feedback very welcome!
And, again, apologies for the suboptimal initial implementation of this feature!
Good to hear, thanks for providing feedback!
Merged.
Thanks again, everyone!
Great, thanks for your feedback!
I now just merged the MR, but if you find the time to create a functional test, feel free to create a new ticket and MR for that and ping me (for instance, via Slack) so we can get it merged quickly.
In any case, thanks a lot again, everyone!
OK, that would of course be great. Thanks for checking!
Can someone else also confirm that this is now resolved with Core versions that include the fix for
✨
Continuation Add Views EntityReference filter to be available for all entity reference fields
Active
? If so, or if otherwise noone objects, I guess we can really close this as outdated.
This merge request contains the code discussed above, disabling/removing automatic migration of settings to the Key module both on update and when newly installing the Key module.
Would be great to get feedback on this, but if the tests pass I’ll probably already merge it to HEAD in a few hours so people can test it using the dev snapshot.
@mgaskey: Thanks a lot for bringing this to our attention! That is indeed a serious concern. I’m working on a follow-up MR that addresses this problem and have marked the 1.9.0 release as unsupported in the meantime to hopefully prevent others from inadvertently stumbling into this situation. Hopefully, we will be able to create an amended 1.9.1 release quickly to allow users to update safely.
Anyways, it seems our implementation here was a bit too aggressive and the underlying assumption that all users of the Key module would want their credentials migrated to Key-based storage incorrect. Probably, the initial approach of giving users the choice of where credentials are stored would have been the safer bet here.
My MR will implement just that: Remove/Disable all code that automatically moves credentials to Key-based storage and also provide an option for users to transfer settings back to config if they have already been transferred (either because they have already updated to 1.9.0 or because they tried it out but then decided against it after all). The plan is to keep this pretty simple for now to get to a fixed 1.9.1 release as quickly as possible – even though this will make the process a bit more cumbersome for users that do want to move to Key-based storage.
In a next step we can then discuss the best way to provide the migration functionality to users that want it without forcing anyone to migrate.
Merged.
Thanks again for your work on this, @solideogloria!
Seems I managed to get all the PHP 8.5 deprecations now, just had a single test I broke with a fix. Should be passing now.
(The remaining deprecations in the “max PHP version” tests were due to Drupal 11.3 deprecations we will fix in
📌
Adapt to deprecation of hook_requirements()
Fixed
.)
Updated with some more fixes and also added a
change record →
for some silly mistake I noticed in a service method.
Let’s see what the test bot says.
It is now released as part of release 1.9.0 → .
The return is needed if you want Drupal’s configs to be used, yes, otherwise you’ll get to the part of the code that discards parameters from the Solr query. If you want to have that, then you don’t need to change the code, you just need to enable the “Configure searches via SearchStudio” setting.
However, if spellcheck functionality isn’t working when that setting is disabled then that would of course still be an issue we need to investigate.
Update: I just created the 1.9.0 release → . Sorry for the delay!
Thanks a lot for reporting this problem!
I could reproduce the issue and was able to identify the cause. This MR should fix the problem. I have already merged it to HEAD, but please give it a try and provide feedback on whether this works for you.
Once confirmed, this will be part of the next stable release.
drunken monkey → made their first commit to this issue’s fork.
This is still being tested by QA. Sorry for the delay!
It will be part of the upcoming 1.9.0 release, hopefully next week.
Yes, this is still being tested by QA. Sorry for the delay!
It will be part of the upcoming 1.9.0 release, hopefully next week.
This was now confirmed to be working.
Thanks again, everyone, and especially @rajeshreeputra!
Confirmed to be working, thanks.
I’m already working on this, and yes, on the whole PHP 8.5 compatibility. No need for a second issue or change of title, and also no need to link the two issues in both directions.
Please let’s not deal with those one at a time.
I noticed a small bug when using the Key module integration: Servers configured using Key storage were not correctly identified as “SearchStax servers” by our utility service (SearchStax::isSearchstaxSolr()) anymore.
Should be fixed in this MR which I’ve just merged to HEAD.
drunken monkey → created an issue.
drunken monkey → created an issue.
drunken monkey → created an issue.
drunken monkey → made their first commit to this issue’s fork.
Thanks a lot for reporting this!
I haven’t completely gone through your steps to reproduce but it does seem quite obvious that this check is pretty useless when we define the 'format_plural_values' option ourselves with default value [].
For this reason, I also think this doesn‘t really need test coverage.
So, merged.
Thanks again!
Yes, this was discussed and decided in
🌱
Decide between events and OOP hooks
Active
: We’re still sticking with events so the plan is still to remove our hooks in a future 2.0.0 release.
Regarding the “6 year” part see
#3126115-6: Create a 2.0.0 release →
: I rarely receive any funding for maintaining the Search API and just don’t have the time at the moment (or in the near future) to undertake the huge project of creating a 2.0.0 release. So, it’s completely unclear when this release, and corresponding removal of our hooks, will actually happen.
Thanks for spotting this, good catch!
I just updated the code to use str_starts_with() and cleaned up the (existing) code style in that if while we’re at it.
drunken monkey → made their first commit to this issue’s fork.
Hm, interesting. The fail against Drupal 11.3 seems unrelated (see
🐛
Update ignored deprecations with latest from Core
Active
) but if this is passing against Drupal versions that do not have the required base class it seems like the test isn’t actually testing the new functionality? I.e., it also passes with the current standard filter plugin?
Or do you have another explanation?
drunken monkey → created an issue.
Not sure, I do think there are other places where this needs to be changed. I see the following other lines on this page:
-
Always prefix types with the fully-qualified namespace for classes and interfaces (beginning with a backslash). If the class/interface is in the global namespace, prefix by a backslash.
-
When documenting variables with the
@vartag note that you should use the special/** */delimiters with a double opening asterisk. Always use the fully qualified namespace of the class or interface, and include the name of the variable at the end.
I’d say those should be changed (or maybe even removed, in the former case) as well.
I also think we should be consistent in whether we hyphenate “fully-qualified” but that might be a different issue. (I see two occurences without a hyphen, here and in namespaces.md.)
The change itself looks good to me, though, so +1 on that.
I would say that
->message(t('Hello @planet', [
'@planet' => 'world',
]));
should be allowed, but definitely not your first example. Another way to allow this would be to say that only the last argument can be split into multiple lines (if it’s an array or function/method/constructor call). (I don’t think continuing with the arguments after the array is a good idea, though I have admittedly done this in test files.)
In any case, we should then probably also specify that the argument’s opening and closing brackets/parentheses should not be preceded resp. followed by a newline. That is, we don’t want this (I’d say):
->message(t('Hello @planet',
[
'@planet' => 'world',
],
));If necessary, however, I’d rather forbid both of your former examples than allow your first example, in case we cannot manage to define this without it becoming unreadable.
The last one I’d definitely want to allow, though I admit it also is tricky to properly define. I guess what we are saying with that is that a nested call/array can be multi-lined even if the enclosing structure isn’t if it is the only argument/element? For the reverse, it should of course (I’d say?) also be fine to have single-lined calls inside a multi-lined call:
a(
b($a, $b, $c),
$d,
);
Maybe easier to guide by examples than to explicitly define in words what is or isn’t allowed?
We have two different proposals now, one patch and one MR, with the MR having an additional change for which I don’t understand the reason. Could someone please clarify what that’s about?
In general, the canonical reference for the proposed changes in an issue is now the MR(s), not any patches posted here which are now just meant as helpers for composer-patches. Please do not test a patch if it doesn’t correspond to the current state of the issue’s MR.
Finally, more people providing feedback on what works or doesn’t work (or seem like a good idea) for them would be nice.
Brilliant, thanks a lot for your work! Looks very good, I just made a few small modifications. Do these look good to you?
In any case, would be great to get feedback from someone else, too, but if no-one pipes up I’d still merge this in a week or two, I think. A functional test would have been nice to have, too, but should be fine as-is. It’s already better tested than most of our Views integration, I think.
Rebased and added test coverage for the new cache plugin as well.
I think this is good to go, but would be great to get one last confirmation before merging.
Brilliant, thanks a lot, @scott_euser!
I made some tiny adjustments and also temporarily enabled testing against other Drupal versions so we can make sure this doesn’t break anything in Drupal versions that do not yet have the EntityReference filter class. (Though, now that I think about it, it is sure to at least break the new test? Let’s wait and see.)
Anyways, apart from that this looks ready to be merged, though some more feedback (tests, reviews) would of course also be nice.
Seems sensible, thanks a lot for your input!
Closed in favor of
✨
Support for entity reference filtering like TaxonomyIndexTid
Postponed
and a future follow-up, then.
It is unfortunately still as relevant as 14 years ago from what I can see: \Drupal\Core\Entity\EntityInterface::toUrl() still does not allow NULL as a return value and the anonymous user still returns a URL that does not exist (or, same difference, no-one can access).
See here:
echo \Drupal\user\Entity\User::load(0)->toUrl()->toString();
// Output: /user/0
This has now been verified to work.
As the pipelines are finally passing I now merged the MR to allow people to test it using the dev release. I ’m still open to any comments and suggestions on this until this is included in a stable release. (And, naturally, for any bug fixes after that.)
Thanks again for all your work on this, everyone – especially @rajeshreeputra, of course!
I now also added an update test. Would still be good to get answers on the two open threads in the MR, but otherwise I’d say this is now ready for testing and review again.
I might already merge it to HEAD tomorrow or some time later this week to make testing easier, but we could still make modifications and fixes afterwards.
drunken monkey → created an issue.
drunken monkey → created an issue.
@scott_euser: Thanks for your work on this!
But yes, I think the preference is on the approach in
✨
Support for entity reference filtering like TaxonomyIndexTid
Postponed
. However, since that issue lacks a SearchApiReference argument plugin, there might still be room for this issue if it is refocused on providing just the argument plugin.
Alternatively, of course, we could close it and add the argument plugin right in the other issue. Not sure whether one or the other makes more sense.
Thanks for attempting to help, @yazanmajadba, but the proper way to move an MR with merge requests forward is to fix the merge conflicts, not to create a new MR. This would have gotten rid of the existing history in the old MR, and made it hard to see the actual changes you made (which introduced code style issues).
Also, still NW for the missing tests. (Apparently I marked the MR as a draft because of that, but I don’t think we do that in general.)
Thanks a lot for the additional feedback. Then I guess let’s finally go forward with this and see what happens.
Merged.
Thanks again for everyone’s work on this!
Thanks a lot for the additional information, the VAPN module is the information I was looking for. Seems that module uses hook_node_access() for access checks, which we cannot generically support in the Search API. The module would need to use Drupal’s
node-specific grants/records access API →
to give us a chance to incorporate the necessary information into the Search API indexes.
As it’s also, at least compared to the Search API, a pretty small module, I don’t think we’ll be adding a specific VAPN integration into this module. If you still need this issue handled, I’d therefore move it to the VAPN issue queue as a feature request with one of two options for achieving compatibility with the Search API module:
- switch the access implementation to use grants and records; or
- add a custom Search API processor to implement access checks using their own system.
The recently merged MRs are currently being tested, once QA give us their blessing I’ll create a new release.
As a contrib maintainer, I’d have appreciated some heads-up before this is changed again and I just find out when merging an MR the next time. Especially since I’ve now already adapted my custom tooling to work with the old new format and am now supposed to change it again (or spend extra time editing the commit message by hand every single time I merge an MR).
A note about this change on the Contribution Record screen, maybe above the formatted commit message, would have also already helped so I don’t have to hunt around various d.o issue queue to find the correct ticket for the change.
Is the format (at least of the first line) now considered stable or should I wait a bit before adopting the new format?
And is there a way of following such changes affecting all issues and MRs without following all Drupal Core issues, maybe a Slack channel?
Some more deprecations and PHPStan errors to fix, too.
Thanks for reporting this issue, @baernhardj! However, I do need one bit of clarification:
Create a node with the access role authenticated and publish it
As far as I’m aware, this is not standard functionality. Normally, you don’t have the option of setting an “access role” when you create a node. Can you tell me which module you’re using for that?
Are there any others with good experiences using MR 154? @abramm?
I had one comment, otherwise this looks great, thanks!
I think this still needs a small edit to make sure lines we touch here are correct afterwards.
However, since the typo was there before, you might also argue that it is unrelated, in which case I would also consider this RTBC.
Thanks for opening in any case!
Thanks for sharing!
Can we mark this as fixed or did you want something investigated? In the latter case, please reopen with more detailed steps to reproduce, or maybe clarifying screenshots.
Thanks for your comments, I now replied.
Will you continue working on this? Otherwise, feel free to hand off to me at any time. I’m just not familiar with the Key module so would need a thorough review for my code afterwards.
Ah, you’re right, thanks for figuring that out! No idea, how it happened, though. Guess I’ll just have to pay closer attention in the future.
Unfortunately, it doesn’t seem to be possible to change the source branch of a merge request, so we’re stuck with this. Sorry.
Anyways, thanks a lot for your help and feedback, this is now merged.
Thanks again, everyone!
Would have been great to get feedback, but I think I’m still happy enough with the code to merge.
@eric_a: Seems we’re currently pretty undecided on that, so seems like a good thing for which to put a standard in place. And probably to use the one from FIG, then. (Even though I personally prefer it with the space.)
$ git ls | grep '\.php$' | xargs grep -hPo '\bfn ?\(' | sort | uniq -c
108 fn (
85 fn(
drunken monkey → created an issue.
Thanks for reporting!
I rebased the MR, please try again.
Yeah, then it should probably be safe to ignore that aspect for now. (Probably, uninstalling the Key module would break other modules/functionality as well.)
Thanks!
I created a merge request for adapting the associated autocomplete search when switching a search view to a new index. Seems like this isn’t your problem anymore, but should save some headache for other users. Needing to delete the autocomplete configuration before being migrating is a) hard to figure out and b) unnecessarily cumbersome. Seems like it would be pretty simple for us to take care of that. (Unfortunately, I think there will be other modules affected, too, with similar problems, and I don’t think there is a good solution except fixing them one by one.)
Your current problem seems like something else entirely, not actually related to the migration module.
For testing purposes, can you try editing your Drupal search server and using a read-only token for the “Read & write token key” setting? You can’t use that permanently, since indexing won’t work, but does it fix the autocomplete functionality? (It seems like the /emsuggest endpoint that SearchStax provides actually only works with read-only tokens, so that would be something we’d need to fix, too.)
Thanks a lot for trying it out, good to hear it worked, and also for reviewing the MR so thoroughly!
I hope I could answer all your questions there, please tell me if you now think the update function’s behavior makes sense or if you would like to change it.
Should now all be fixed and I also added an update test.
Please test/review.
And yes, very strange that the MR is not linked. It’s also not linked from the contribution record. Seems like something went wrong there.
Anyways, should not really hamper us.
I added the update hook and adapted the existing test. Not sure if any more test coverage is really needed – potentially for the update hook, but that’s not very complex (and a pain to test).
In any case, feel free to already give this a try or review my code.
I have something working now in this draft MR which can already be tested in case anyone is interested.
It’s still missing the update hook and test coverage (probably also test adaptions, so I expect failures there), but the basic functionality should be there. Just make sure to manually set the new auto-suggest core setting on all servers, then you should be able to use autocomplete with all of them.
There is also a
change record →
, feedback on that would be also welcome.
I should be able to complete the missing pieces by the end of the week, or possibly already tomorrow – but thought I’d share what I have right away in case someone has feedback on the basic direction.
Thanks for reporting this problem!
However, (1) should actually never be the case (unless there is a very long word/token), and for (2) I’d need an example – it might be on purpose, but that’s hard to tell with this little information.
Would you be able to add failing tests for this to either HighlightTest (or HighlightExcerptTestif that’s easier)?