Vienna, Austria
Account created on 15 November 2007, over 17 years ago
#

Merge Requests

More

Recent comments

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Seems like mxr576 accidentally set this to RTBC in #15, so setting back to NW. I’m leaning towards ā€œClosed (won’t fix)ā€, though – the ddev lead developer saying it’s a bad idea sounds hard to beat.
If you can come up with a single file that you’d say is almost as useful as your current proposal (as mkalkbrenner suggests in #14), and if it’s easy enough to maintain I might still consider it, but would still need stronger support than now.

ensures all contributors run the same clean, isolated, and predictable environment.

I don’t plan on using ddev for my local contrib dev environment, so that’s actually more a con than a pro in my books.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Does anyone else have any input on this?

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks for posting the patch!
Can someone confirm that the patch resolveds the problem for them?

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

@ravi shankar karnati: Can you confirm the patch works for you, then?

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Currently not, no. At the moment I have trouble enough finding the time to do the bare minimum contrib work to keep my modules maintained, so I don’t want to start such a larger task at this point, without any pressure.
Though I would love having a 2.0.0 version, I can neither spare the time right now nor deal with the inevitable maintenance/support headaches that will surely follow.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks for reporting this problem!

I wasn’t aware of the language parameter that seems to exist for both /emsuggest and /emselect and apparently also causes issues by its absence in the latter – for instance, synonyms were not working as expected for the non-default languages.

All of this should be fixed in this merge request. (It depends on this MR from ✨ Improvements for the ā€œRelevance modelā€ option in SearchStax Solr views Active since both require calculating a ā€œcurrent languageā€ for a search query.)

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Confirmed to be working.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

This is now merged, but still open for comments in case anything doesn’t work as expected.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

I didn’t see this in time and created a duplicate issue for this in the SearchStax module ( šŸ› Spellcheck not working Active ), sorry. The problem is that (if the user has selected that option) we want SearchStax to handle the spellcheck functionality and therefore remove the Spellcheck component from the Solarium query. However, Solarium apparently checks for that when parsing the response so we then don’t get the Spellcheck component in the response object even though there is a spellcheck key in the response JSON.

I expect that that was a deliberate decision when writing the Solarium library so this is likely not gonna change. Therefore I thought the simplest solution would be to ā€œtrickā€ Solarium by re-adding the Spellcheck component after the search request was sent but before the response is actually parsed (\Solarium\Core\Query\Result\QueryType::parseResponse()).
But if we could have a solution in Solarium or the Solr module that would of course be even better, it’s a bit hack-y this way.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Should be fixed with this MR.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

drunken monkey → made their first commit to this issue’s fork.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

@jrb: That wouldn’t be possible with the current code, you’re right.
What would make a separate processor a bit awkward is that this code is actually not in the processor plugin but in parts of the framework code. (Which is not ideal, architecturally, but couldn’t be helped.) So we’d be adding a completely empty processor plugin class for this. Still, the user doesn’t care about that, of course, and if there are some people who want only one half of this functionality it makes sense to split it.
Alternatively, we could add config to the ā€œEntity statusā€ processor to allow users to enable/disable the functionality for the indexed items and for referenced entities separately. Maybe that would make more sense?

One thing we have to take into account in this decision is the experience for sites that are already using the ā€œEntity statusā€ processor and are unaware of this issue. (And which even have references to unpublished entities.) Should the behavior change for them without notice or do we require them to change a setting somewhere to get this new behavior?
Or, asked differently: Will most affected users consider the current behavior a bug (that they might be unaware of) or desired?

In any case, I guess we should also add a change record to make site owners aware of this change. And I’ll try to remember to mention it in the release notes.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Merged. Should be fixed in the dev version of the module.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Merged to the dev branch, might further refine the feature in follow-ups.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Do you mean it appears when importing an index config where you have server: null and status: true?
That is an invalid config, so I don’t think we can be held responsible for any errors that occur consequently.

I think we need test coverage for this to proceed.
Right now, I cannot even see how the original problem would occur without config overrides being present. And if they are present, see above: I don’t think we can be made responsible for people forcing invalid config. Then the only real bug here would be that server: '' makes no sense.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

@mxr576: Thanks for bumping this, seems this slipped through the cracks.
However, on reviewing the MR, do we maybe also need to make a modification to SearchApiTimeTagCache, which was added in the meantime (see ✨ Add a third Views cache plugin (tag- and time-based) Needs work )?
Other than that, I agree, this is ready to be merged.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks a lot for your work on this, much appreciated!

According to this change record → the Views plugin types were converted to attribute discovery in Drupal 10.3 so I think we need to depend on that in order to drop the BC annotations right away. However, since 10.2 is already unsupported (unless I’m mistaken it even seems like support for 10.3 was just dropped) I think this should be fine.

Surprisingly, the failing test is related: Switching our Views row plugin(s) back to use the @ViewsRow annotation does fix the test. The functionality tested is implemented in search_api_search_api_index_insert() but it seems the call to \Drupal::getContainer()->get('plugin.manager.views.row')->clearCachedDefinitions() does not work when using an attribute instead of an annotation. Not even sprinkling further such calls throughout the code (e.g., in the test method itself right after creating the new index, or in \Drupal\views_ui\Form\Ajax\Display::__construct()) fixes the test.
On the other hand, the functionality seems to work fine when trying it out manually (adding a new index and checking whether you can set its row style to ā€œRendered entityā€ without a manual cache clear) so this seems to be a tests-only problem, which likely would have to be fixed in Drupal Core.
However, I now spent several hours of precious contrib time on this and don’t have time to file a Core issue with the necessary information and (ideally) failing test case. Leaving this for either anyone else to take care of or for me to later pick up when I have the time. For now, I guess we have to leave the workaround in place to make pipelines pass.

The final remaining deprecation warnings regard our own plugin types and their lack of attributes. I added attribute discovery for backend plugins but this also was a bit of work so I had to leave it at that. If noone else currently has the time we can ignore those warnings for now and add a follow-up issue.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

When running composer install from inside the Git checkout of this module you’d also need to add the repositories entry for the Drupal Packagist repo, but yes, I then also had to add the branch alias for it to work.

While I’m again not familiar enough with Ddev to confidently say whether this change works as intended or is even needed, it seems harmless enough and does fix at least something, so it should hopefully at least not cause any problems. I therefore merged it, hopefully it will help a few people.
Thanks again!

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

OK, thanks for the detailed explanation!
I will still leave this open to be reviewed by others since I know too little about it to confidently say whether this makes sense or not. But I see no problems so far.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Should be fixed in this merge request, I’ll merge it if the test bot is happy.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Marking this as fixed for now, might re-open or create follow-up issues in case any problems emerge.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Implemented in this merge request.

As far as I can tell, only the /admin/info/system endpoint is blocked for SearchStax apps that use Basic Auth, and all others need to use our custom connector plugin anyways, which already takes care of bypassing all requests to blocked endpoints.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Looks good, and seems small enough of a change to be acceptable even if it probably won’t be useful to more than a few people/modules.
I made some minir changes to the MR, please review. Also the newly adapted test is failing, my guess would be that the index instance use in the indexing batch is not the same as in the test method.
If you can think of another quick way to test the new method, that would be great, but otherwise I’d say it’s also no big deal if there is no test coverage, as long as you’re confident it works as desired (as the main user of the new method) and think it’s unlikely it will ever break due to some changes in Core or our batch indexing code.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

drunken monkey → made their first commit to this issue’s fork.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

I think this looks good now, thanks!

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Merged to the dev branch, will test from there.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks a lot for suggesting this improvement!
I must admit I’m not too familiar with DDEV so am a bit overwhelmed by this MR.
Could you please tell me some examples of other modules that have already done this? Do you have links to some issues that discuss this change?
Adding a whole new folder with 19 files seems like a pretty big change. What exactly is the advantage of doing this, seems like most of this is auto-generated? (In fact, could you please tell me which parts are not auto-generated? Is it just the first few lines of .ddev/config.yaml? If so, wouldn’t DDEV be able to generate the rest anyways, without us including it in the repository?)
Also, what exactly is the use case for which this is needed? Is this for including Search API in any website project, or just for testing Search API on its own in a test environment?
Finally, why did you choose MariaDB and specifcally version 10.11? Isn’t hard-coding that in our module repo a problem?

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks for reporting this issue and already providing an MR!
Unfortunately hard to test whether this actually works … How confident are you in your solution? Any examples of other projects where this worked/helped?

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Looks perfect, thanks for spotting this!
Merged.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

OK, this seems to work and be backwards-compatible, just like the change record → promised.
Now we just need to go through around 40 other plugins and make this change.
Anyone interested in a simple, if slightly annoying, issue credit? Otherwise I’ll try to get to it when I have more time (and might just silence those deprecation warnings in the meantime).

Regarding the Postgres failure: unfortunately that seems to be persistent. Does anyone have any idea what could be going wrong there?
In the schedule config, I’m setting _TARGET_DB_VERSION to $CORE_PGSQL_MIN – is that maybe wrong?

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Hi, and thanks for reporting this issue!
However, I don’t think I’ve heard something like this before and can assure you that this works in general, of course, we have extensive test coverage to prove that. Therefore, I’m not sure what I can do here to help you, I don’t really have suggestions on what to try.
Or, if you’re trying this on your existing site, maybe try with a bare installation of Drupal? If that works, try to change one thing at a time and see when it breaks. But a lengthy process for sure, sorry.

Maybe someone else has run into this before and can jump in here.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

The Postgres fail looks like it’s just a CI problem (database not running), nothing to do with us. I’ll run the pipeline again.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Let’s see whether just adding the attribute works with all Drupal versions. Otherwise, we’ll probably have to just ignore those errors for now, until we depend on the relevant Drupal Core version.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

drunken monkey → created an issue.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

That is a good suggestion, thanks! It’s also the decision we reached in ✨ Allow excluding unpublished references from index Active .
However, could you maybe check whether the MR there already covers this use case, too? It would be better to just solve this issue once for the whole field extraction code. And we’re missing test coverage in either case. (Otherwise, your MR looks good, thanks!)

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

I think this is fixed now?

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Seems reasonable.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks for suggesting this improvement! I agree that in theory it makes sense to add this parameter and pass the plugin definition to supportsIndex().

However, there is of course a large problem when adding a new parameter to an interface, namely backwards-compatibility. We’d have to add it in a commented-out way (see #3128548: Add optional parameters to StatementInterface::fetchObject() to be in line with the PDO implementation of the method fetchObject() → for an example from Core), adapt our implementations and hope that other classes implementing the method will also do that before we change the signature for real in a future 2.0.0 release of the module. (Based on a sentence in the IS of šŸ“Œ [11.x] Adjust parameters in interfaces Active , which discusses making that same Core change permanent, it seems like Symfony’s debug classloader will understand this syntax of commenting out parameters, so people might at least get notified.)

While it seems there is still a clear way to do this, it does carry the risk of breaking people’s code when we release version 2.0.0, and I’m not sure the use case you describe is common enough to warrant this effort and risk. I’m keeping this open for now in case others would be in favor of this change, too, but am currently sceptical about whether we should do this.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks for reporting this issue!
I was not aware of this change, but definitely seems like something we want to support, indeed. I was always a bit peeved that node access was so inconsistent and giving users access to some unpublished nodes always required a bit of magic.
Seems it didn’t really get more consistent, but the system at least now has clearer rules and less magic, which is also nice.

However, needless to say that this is a very sensitive area and we definitely do not want to introduce a bug that would show visitors content to which they do not have access. And since the change was only introcuced in Drupal 11.2 I wonder if we don’t need to also add a version check to make sure we don’t apply this new logic pre-maturely, i.e., in versions of Drupal Core that don’t apply it themselves yet.

It seems like our current test coverage would not catch this problem, but we should make very sure to not introduce any security problems, even in edge cases.
More worryingly, the tests even seem to fail against Drupal 11.2 currently, so please check that out, too.

In any case, thanks again!

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

drunken monkey → made their first commit to this issue’s fork.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

drunken monkey → made their first commit to this issue’s fork.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Huh, you’re right, good catch! Then I think that is the real bug here, the problem in Index::postSave() is just a symptom. ā€œNo serverā€ should definitely be represented as null, not ''.
I think we should make sure an index is never saved with '' as the server ID and probably also provide an update hook that sanitizes existing indexes.

Would it be OK with you if we converted this issue accordingly, or would you prefer a new issue and would be in favor of still adding this check (which I’d argue against, but am prepared to be convinced/outvoted)?

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Good to hear, thanks for reporting back!
Merged.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Looks good. Merged.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

drunken monkey → created an issue.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Seems the fails unders Drupal 10 are in HEAD already, and it otherwise seems to work well, so merged.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Didn’t work either, so let’s just fix this in the Search API Page module as we should have done from the beginning. See šŸ› Fix deprecation warnings in PHP 8.4 Active .

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

OK, that didn’t work either.
Seems like the test environment maybe just doesn’t understand the @dev version specification? Let’s try that …

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Seems one potential fix here would be for the server backend plugin to include empty fields in their result items if they tried to retrieve the field, since this means that the item was indexed without a value for that field and explicitly returning the field as empty makes sense.

However, it also makes sense to have a similar setting for the excerpt as for highlighting individual fields, for not loading field values from the database if not already included in the results. So, if you want to provide an MR for that, I’d be open to including this functionality. (Just ā€œrespectingā€ the existing setting would break sites relying on the existing behavior so I don’t think we can do that.)

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks for reporting this problem!

As you already saw by the test failure, the Index class uses $this->datasourceInstances as the ā€œsource of truthā€ for its datasources. Keeping this consistent helps avoid a lot of thorny problems when changing index settings (see #2638116: Clean up caching of Index class method results (especially fields) → for background on this decision). For instance, a similar change as the one for removeDatasource() would have to be made to addDatasource() or getDatasourceIds() would return incorrect data when called after adding a datasource.

However, I guess this doesn’t really apply when the datasource plugins aren’t loaded yet, so maybe using either $datasourceInstances or $datasource_settings based on whether the former is initialized would work in all cases. The only ā€œbreakā€ in functionality would now be that Index::getDatasourceIds() will never throw an exception, and since that exception wasn’t even documented I guess this is acceptable. As you say, it might even improve performance in rare cases.

Please give the new code in the MR a try!

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

drunken monkey → made their first commit to this issue’s fork.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Shouldn’t the server be set to null instead of ''? The former means ā€œno serverā€, the latter looks to most code like a server with ID ''.
In your setup, there also seems to be something else going wrong as an index without server cannot be enabled.

If you try to import invalid configuration it is clear that errors will occur, I wouldn’t say that is a bug in the code.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

What exactly was the schema error. The default is search_api.default_processor_configuration, so if you needed to add search_api.fields_processor_configuration then maybe your processor inherits from FieldsProcessorPluginBase? In that case, it is expected that you would need to specify that explicitly, we have no way of detecting in the config schema whether a processor inherits from that.

For those processors that just inherit from ProcessorPluginBase it seems to work fine. For instance, there is no explicit schema for the entity_status processor.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Seems changes to test_dependencies might only take effect after they are merged, not when just inside a fork, based on the docs → . So, in the spirit of experimentation, I just merged the MR. Let’s see what happens.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Seems the deprecation warnings were actually already fixed in Facets, see šŸ› Deprecations PHP 8.4 Active . We are just not using the latest dev version of the module, and there hasn’t been a stable release that includes that fix yet.
Let’s see, maybe I can get the CI to use the dev version of the module after all.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Thanks a lot for writing the test, looks great!
I just made a few minor changes, now I’d say this is RTBC.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Great to hear, thanks for the feedback!
Merged.
Thanks again, everyone!

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Running this locally with XDebug enabled makes it immediately clear why PHPUnit fails to finish: The current code actually leads to an infinite loop, as the default method implementation \Drupal\search_api\Backend\BackendPluginBase::setConfiguration() actually calls $this->server->setBackendConfig(). Our current code actually already guarded against this, so that the config stays in sync no matter which method you call but you should still never get an infinite loop.
So, let’s just remove one condition from the if check and add the try/catch and that should fix all that.

Please test/review again!

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Sorry if this is completely off-topic for this issue, but I’m unclear why setting a custom ignore file for deprecations via SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=…" doesn’t work to prevent those failures in PHP 8.4 anymore? E.g., in this MR I added such a file for ignoring deprecation warnings triggered by a different Drupal module (since it makes no sense to fail on those), but the ā€œphpunit (max PHP version)ā€ job still fails because of output caused by the deprecation warning.

If there is really currently no way to avoid failing on deprecated code in other contrib modules (except to disable larger swaths of checks such as tests printing output) then I’m very much in favor of adding some mechanism for that. Though I’m not sure whether the proposed changes would actually allow for that or just help you disable failing on output.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

There are currently no PHPCS failures in this module, see the scheduled pipelines.

šŸ‡¦šŸ‡¹Austria drunken monkey Vienna, Austria

Agreed that it’s important to keep this from becoming common, but probably already covered by the linked documentation. Let’s just adapt our tools to spot this.

Production build 0.71.5 2024