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

Merge Requests

More

Recent comments

🇦🇹Austria drunken monkey Vienna, Austria

I don’t think it’s necessary to generally enable testing against PHP 8.4 for all MRs and commits, that seems like a waste of money. I do already have a weekly pipeline that tests for PHP 8.4, I now also added one for Drupal 10 and PHP 8.1, as suggested – thanks.

🇦🇹Austria drunken monkey Vienna, Austria

This should already work as you describe, see Index::reactToProcessorChanges() and ProcessorPluginBase::requiresReindexing(). Can you give me more detailed steps to reproduce?

🇦🇹Austria drunken monkey Vienna, Austria

Thanks for reporting this issue! It would indeed be nice to not load those Javascript files unless they are needed.
However, Javascript isn’t really my area of expertise, I wouldn’t know quite how to implement this suggested solution. Any MRs would be welcome, though!

🇦🇹Austria drunken monkey Vienna, Austria

Thanks a lot for your feedback, sounds good!
Merged.

I also created and published a change record .

Thanks again, everyone!

🇦🇹Austria drunken monkey Vienna, Austria

Since last week’s scheduled pipeline runs there was also one more change that affected us, the deprecation of entity_test_create_bundle() .

Should all be fixed in this MR.

🇦🇹Austria drunken monkey Vienna, Austria

Great to hear, thanks a lot for testing!
Merged.

🇦🇹Austria drunken monkey Vienna, Austria

Thanks a lot for weighing in on this, everyone! Good to get feedback on this decision.

It’s not a lot of voices so far, but seems there is a strong preference for sticking with events. I’m gonna let this open for a bit more and then mark as fixed. We are free to revisit this in case there are further developments.

@xen: I don’t think that OOP hooks using events internally is really an argument, since that’s immaterial to anyone using or defining them. It’s more an implementation detail and doesn’t make them any more in line with general PHP best practices.

🇦🇹Austria drunken monkey Vienna, Austria

Created an MR.
Note that this would only add the latency key for Solr searches, though, not when using other search backends.

🇦🇹Austria drunken monkey Vienna, Austria

Thanks for the feedback, good to hear this works.
Added my tiny change from #6 to the MR and merged.
Thanks again, everyone!

🇦🇹Austria drunken monkey Vienna, Austria

I think I would rather disallow type-hinting with \stdClass at all to resolve this, then it not being allowed in the PhpDoc would not be a problem. Type-hinting on \stdClass seems like bad practice to me and it should be discouraged if it isn’t yet in our standards.

Anyone know what 'stdClass' was excluded?

Seems like this was written in a time when Drupal mostly used anonymous objects and we just wanted to document those as object in the PhpDoc, not as \stdClass. Actual type hints were not allowed in PHP at that time, so the question of a discrepancy between PhpDoc and in-code type hint never arose.
And we probably couldn’t think of a good reason to actually require a \stdClass object instead of any object with the required (public) properties – same as I do now.

🇦🇹Austria drunken monkey Vienna, Austria

@stephenplatz: Does the patch in #25 work for you?
Otherwise, what error and stack trace do you get?

🇦🇹Austria drunken monkey Vienna, Austria

Thanks for the suggestion! However, I’m not sure I’m convinced.
In theory, it seems like a good idea to separate the rules for the two, but then the extra “Acronyms in a Class or Method name” section makes it awkward. You now have to read two sections for the naming conventions of a specific entity instead of one, which might make it easier to miss. It also looks a bit silly to have a section with just a single sentence.
“Stand-alone name examples” being a sub-section of that also doesn’t make much sense, I guess it would make more sense to bump it up to an <h3>? Or maybe all four should be <h4> under a new <h3>Object-oriented code</h3> heading (or similar)? The first half of the example could even be nested under “Classes and Interfaces”, as it only lists those.

All in all, to me it seems like this creates more problems than it solves, tbh, if only because of the acronyms rule and the example code that is pertinent to both classes and methods. (Also, I guess the acronyms rule even applies to properties? Would be strange if it were XmlSource and getXml() but $extractedXML.)

🇦🇹Austria drunken monkey Vienna, Austria

Oh, darn, it actually seems like this is a bug in Drupal Core: I created 🐛 TypeError when having exposed form in block and setting "role" attribute on it Active to address it.
As this not only affects the tests, but can actually be reproduced on an actual site if you have an exposed form in a block, I fear we cannot actually merge this until the Core bug has been fixed and we depend on a version of Drupal Core that contains that fix.
I unfortunately don’t even think we can really work around this problem in this module without an unacceptable amount of code.

🇦🇹Austria drunken monkey Vienna, Austria

That’s a very good point, thanks. I opened 🌱 Decide between events and OOP hooks Active to discuss this for the Search API module, this module should then probably just follow suite to remain consistent at least within the Search API ecosystem.

🇦🇹Austria drunken monkey Vienna, Austria

Now that Core supports OOP hooks the question is whether we want to keep moving towards the use of events, or reverse course to embrace hooks once again.
Please help with your input in 🌱 Decide between events and OOP hooks Active .

🇦🇹Austria drunken monkey Vienna, Austria

In principle, I think the "not empty" check is preventing ambiguousness and should be favored. Only with the result count I am in doubt if a value of "0" should be considered empty or is expected to be empty? But is it possible at all to have a result count "0" and appear in the autocomplete suggetions at all?

It’s admittedly unlikely, but since the value can be NULL it should be assumed that 0 represents a valid count that should be reported, I think. If anything, I’d just stick with this for the sake of consistency.

I merged the MR now. Thanks a lot again!

🇦🇹Austria drunken monkey Vienna, Austria

Great to hear, thanks for your feedback!
Merged.

🇦🇹Austria drunken monkey Vienna, Austria

Great, thanks a lot for reviewing!
Merged.

🇦🇹Austria drunken monkey Vienna, Austria

Thanks for opening this!

The current documentation on reads:

Classes and interfaces without a backslash \ inside their fully-qualified name (for example, the built-in PHP Exception class) must be fully qualified when used in a namespaced file. For example: new \Exception();. Do not use global classes.

It seems we’d just need to remove the “when used in a namespaced file” part?

Adding myself as a supporter. Also fixing the title – this is not about function calls but about references to classes (for static method calls, constructing new objects or just the ::class reference).

🇦🇹Austria drunken monkey Vienna, Austria

The code in SearchApiFieldTrait is unfortunately very complex, but in theory there should already be code that ensures that fields will only be extracted if they are not yet set on the result row – see the code determining $required in \Drupal\search_api\Plugin\views\field\SearchApiFieldTrait::getValuesToExtract().
But it’s entirely possible that there are holes in that code. Still, as it’s probably rather specific to your setup, it would be great if you could determine where the code is going wrong and which additional checks might help.

I don’t think we need a new option for this, as this is already what we’re trying to do. It’s just fixing the existing functionality.

🇦🇹Austria drunken monkey Vienna, Austria

Thanks for reporting this problem!
That’s really a doozy. Off the top of my head, I can’t venture a guess why this would happen.
Are you able to obtain a stack trace for the error? (It is thrown in \Drupal\search_api\Entity\Index::getDatasource().) That would help a lot.

(In the future, however, please don’t post config YAML as a comment, but instead use an attachment.)

@w01f: That seems like an entirely different problem, namely that you have Search API installed via Git and Drupal unfortunately does not handle that correctly. It therefore incorrectly reports a dependency version conflict.

🇦🇹Austria drunken monkey Vienna, Austria

It is known and unfortunate that Paragraphs and Search API don’t play very well together.
I usually suggest better using just a “Rendered HTML output” field for making the text inside Paragraph entities available for searches – however, in your case it seems you have already done that and it didn’t work.

The first thing I want to note, though, is that you usually shouldn’t add the “Paragraph” datasource to search indexes. This would only make sense if you want the Paragraphs themselves as search results – usually, however, users want the results to be Content items, and the Content’s Paragraphs included in the indexed fulltext for the content. In that case, you should just use the “Content“ datasource and add fields that point to the fields of the Content’s Paragraphs that you want indexed. (Or, as above, use the “Rendered HTML output” field.)

That being said, indexing Paragraphs directly should also work, so it’s still unclear why that search wouldn’t work.
To debug whether the problem is during indexing or during searching, please look into your database, in the search_api_db_default_index_text table. This contains all words indexed for the “Default content index”. By filtering for field_name = 'field_accordion_title' you should see all the words indexed for Accordion titles.

Anyways, indexing specific fields of Paragraphs is generally not recommended as it is very fragile wrt reorganizing of the Paragraph structure. (When you put that Paragraph into another one, the field will suddenly not be found anymore.) The “Rendered HTML output” field is generally the better solution, if correctly configured (to not include field labels, etc.). But debugging why that doesn’t work in your case is probably even harder, you’d probably need to add debugging code (or an XDebug breakpoint) to \Drupal\search_api\Plugin\search_api\processor\RenderedItem::addFieldValues() to see what’s going on/wrong there.

In conclusion, this is a pretty site-specific problem so I don’t think I can help you any more than this. You’ll have to debug this yourself, one way or another.

🇦🇹Austria drunken monkey Vienna, Austria

Thanks for suggesting this improvement!
You are right that I’d say adding explicit support for Paragraph entities would be a bit too specific a solution for my taste. So, making this easier for custom code to fix on its own seems like the better approach.

I wanted to comment and say that we’ve moved to using events instead of hooks and to please create one of those instead – however, I then realized that we haven’t actually made that move in this module. I created 📌 Convert all hooks to events Active for that – in case this gets merged, we’d just have to port one more hook to an event. (Though it seems less than ideal to introduce a new hook and then perhaps deprecate it a few weeks later.)

However, introducing a new hook for that also seems like a bit of an overkill, at least unless more people express interest in this functionality.
A simpler approach would be to extract the $datasource->getItemUrl($object) call into its own method on the LiveResults suggester – that way, you could simply change the plugin’s class to a custom subclass where you override that method.
Would that also work? It seems to me like that would be a less invasive solution to the problem, even though a bit more custom code would be needed as a downside.

🇦🇹Austria drunken monkey Vienna, Austria

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

🇦🇹Austria drunken monkey Vienna, Austria

*bump* Please test/review so I can merge this.

🇦🇹Austria drunken monkey Vienna, Austria

Would be great to get a) feedback and b) help on that test. It has to be something stupid, probably because I’m unused to working with translations – maybe someone more experienced there can quickly spot my mistake?

🇦🇹Austria drunken monkey Vienna, Austria

I changed my mind, let’s keep compatibility and get rid of the AutowireTrait.
Changed the MR, please test!

🇦🇹Austria drunken monkey Vienna, Austria

Merged.
Thanks for bringing this over the finish line, Scott!

🇦🇹Austria drunken monkey Vienna, Austria

Great to hear, thanks for reporting back!
Fixed the conflict and merged.
Thanks again!

🇦🇹Austria drunken monkey Vienna, Austria

@capysara: Excerpts are already created without any HTML (except for the highlighting tags, of course) by the Highlight processor, so there is nothing more to display for the view. By switching to “Do not allow HTML” you could just make the <strong> tags (or whatever you have configured) around matches visible.

If you want HTML preserved inside the excerpt you’ll need to implement your own code for creating the it and use that instead of the Highlight processor.

🇦🇹Austria drunken monkey Vienna, Austria

Merged this. I’d consider this experimental for now, I’ll create a new release once the feature has been tested thoroughly.

🇦🇹Austria drunken monkey Vienna, Austria

This is now implemented and tests are passing locally.
I based the MR on the MR for the Migration module to avoid later merge conflicts, so that one will have to be merged first before merging this one. Still, testers already welcome!

🇦🇹Austria drunken monkey Vienna, Austria

To be on the safe side, I re-added the constructor with a deprecation warning in case it is called with any parameters. That way we will know to handle this properly in case we ever add a new parameter.

🇦🇹Austria drunken monkey Vienna, Austria

@recrit: Thanks a lot for your review, both your comments make a lot of sense. I now amended the MR accordingly, please review!

Luckily, we didn’t even publish a change record about the new required argument for AutocompleteHelper, so hopefully just removing it will be fine. (Anyways, removing a parameter in general seems pretty harmless – unless we add a new one in the future.)

🇦🇹Austria drunken monkey Vienna, Austria

Could I please get a few more testers for the MR?
With 63 followers there should be a few willing to bring this over the finish line, right?

(And just to be on the safe side: The whole purpose of the MR is to retry indexing during the next cron run if this error occurs during the node/entity save operation. It won’t change anything about that initial fail, except for downgrading the log message to a warning, and also won’t help if indexing the rendered content still fails during the cron run.)

🇦🇹Austria drunken monkey Vienna, Austria

Still didn’t work.
Well, those should be resolved by the other modules anyways, so we can live with them for a week or two.

On the other hand, I now tried to fix the reported deprecations against Drupal 11.2 for using the deprecated Entity::$original property. Let’s see how that goes.

🇦🇹Austria drunken monkey Vienna, Austria

This will now be mostly resolved in 🐛 Fix failing pipelines Active , but once we depend on Drupal 11.2 we can remove the DeprecationHelper::backwardsCompatibleCall() calls.

🇦🇹Austria drunken monkey Vienna, Austria

I have to admit that I don’t really understand this whole problem, or the whole mechanism at work here. Why did Core remove those words, and why does this not cause CSpell errors in the Core pipelines?

However, from a contrib perspective it seems clear that this is a regression and at least some of those words should be added back. (Though, by the above, I have no idea whether to do this in Core or in this project.) Looking through your list (Thanks!) I see lots of words that are actual nonsense, for which it makes sense to remove them and let contrib deal with any instances of them in their code.

However, a few words I’d say are common enough (some are even on Wiktionary, I linked them below) to warrant re-adding and I think I’d be confused if any of them were suddenly reported as spelling errors:

That being said, it might be the case that some of those are now just part of the inbuilt CSpell dictionary, or something like that, so they don’t need to be included anymore? I quickly tested in the Search API module and this indeed seems to be the case: only “bodyless”, “kernighan”, “permissionless” and “varchar” are actually reported as errors.
I would suggest readding those four words in some way but, again, I don’t actually know where to best do that.

🇦🇹Austria drunken monkey Vienna, Austria

Sorry about crossposting, I really didn’t think anyone pick this up in the few minutes between me posting this and then creating the MR.

🇦🇹Austria drunken monkey Vienna, Austria

Great job, thanks a lot!
Unfortunately it’s not that easy anymore to test against other databases (after the switch to GitLab CI) but tests against MySQL pass and any problems with Postgres or SQLite should be reported on the weekend when the scheduled pipelines run.
I just did a bit of code cleanup and will now merge.
Thanks again!

🇦🇹Austria drunken monkey Vienna, Austria

Great to hear, thanks for reviewing!
Merged.

🇦🇹Austria drunken monkey Vienna, Austria

Thanks, very helpful! Then yes, it seems clear that this is the right thing to do.
Merged.
Thanks a lot again!

🇦🇹Austria drunken monkey Vienna, Austria

Merged. Thanks again!

🇦🇹Austria drunken monkey Vienna, Austria

Huh. No idea why that didn’t work.
Especially: Why did the PHP 8.4 tests not report the newly added implicitly nullable type?
Trying again with a different regex.

🇦🇹Austria drunken monkey Vienna, Austria

Seems these are just deprecation warnings caused by other contrib modules. We can just ignore them here, they should be fixed in the respective modules (see 📌 Fix implicitly nullable parameter type hints Active and 🐛 Fix deprecated implicitly nullable types Active ).
I created an MR, let’s see if pipelines pass now (resp. report the one new deprecation I added for testing purposes).

🇦🇹Austria drunken monkey Vienna, Austria

See this MR.

I tested, but it seems like <script> tags are already stripped even in the overview, so this shouldn’t be a security issue.

🇦🇹Austria drunken monkey Vienna, Austria

After several problems just getting the tests to run, and with consistent results, in Drupal 8, pipelines are now passing.

🇦🇹Austria drunken monkey Vienna, Austria

Since it will hardly be possible to wait until the two bugs are fixed and we depend on the versions that contain those fixes, I now instead refactored the code to use a key-value store instead of third-party settings to store the migration history. This also makes for simpler code, so it’s a win-win.
Now just trying to get the pipelines green …

🇦🇹Austria drunken monkey Vienna, Austria

The module is now working well and has test coverage. We also broadened its scope, it can now be used to migrate from any other Solr hosting provider, not just from Acquia Search.

Apart from the failing CI pipelines, there are two remaining problems that are unfortunately caused by bugs in other modules (unless I’m mistaken):

🇦🇹Austria drunken monkey Vienna, Austria

My trivial suggested solution is implemented in this MR.
Let’s see whether the pipelines succeed, otherwise we might already know the reason why this is currently not done.

🇦🇹Austria drunken monkey Vienna, Austria

Thanks a lot for reporting this problem and already proposing a fix!
It indeed seems like we misunderstood the purpose of the version key when initially creating the search_api.libraries.yml file. Your explanation makes sense and the code in JsCollectionRenderer::render() seems to corroborate it as well. Still, as I have very little expertise in this area of Drupal development I’ll leave this issue open another week or so to give others a chance to weigh in before merging.

🇦🇹Austria drunken monkey Vienna, Austria

Thanks for reporting this, @heyyo, and thanks a lot for creating an MR, @prem suthar!
However, there were quite a few spots missing. Fixed those as well in the MR, please test/review and I can merge.

Production build 0.71.5 2024