@admirlju

Rebased against the latest release, DrupalCI is passing but for some reason, GitlabCI is not. And I'm not sure it's failing because of changes in this code. Could someone check this? For that reason leaving it as needs works.

If the support for D9 is dropped. The module page should also be updated. At the moment it still shows support for ^9.3 || ^10.0.

When it comes to the issue, testing shows improved performance. RTBC +1.

Hi, so this is the default behavior of entity type form display when you add a new field. And how it currently works it's probably an issue somewhere in the Drupal core.

If you want to move the links above the save and delete buttons.

1. Edit the media type.
2. Go to Manage Form Display
3. Move the Link field just below the image field
4. Save changes

Yeah, I agree about the README. I'll add the steps to it. I just first wanted to make sure it works like it's supposed to.

Tested it and this fixes the problem. Also, the uploaded patch file is passing automated tests. RTBC.

Haven't done any code changes. Just rebased MR 90, so it is quicker for maintainers to merge the feature. Testing locally it also looks like everything works like it's supposed to and automated tests are passing. Marking this as RTBC.

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

I'm finding it hard to replicate the issue. Could you check what tags do you have set for this specific view, that throws this error? From the error I assume it's some data, that contains a date in it.

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

If you find patching easier, you can generate a patch on the merge request page. So on the merge request page, if you click the button "Code", you'll see the patches option under Download. This will download a patch file, that you can apply.

Created an issue fork, and applied the patches. Locally the tests are passing, but because Drupal CI is deprecated, they will not pass with that. So I just added a basic gitlab-ci. And from what I see they are passing there. Still because of some CS fixes I'll leave this as needs review.

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

Rebased and made a small CS fix.

Also, elements that update the preview on blur, open a preview pane that is empty and use the wrong UUID. So this is a bigger problem in general.

So I've been digging a bit more. I'm not yet sure, but a few things I noticed, might help debug further.

So I'm looking at the module file, and how the iframe and preview compiling is added in same_page_preview_form_node_form_alter.
Noticed one thing, when the preview button is clicked but the pane is not open, this hook is called twice, the first time when it opens the pane, the UUID of the preview is wrong, and the second call uses the correct UUID and then displays the data. Now the weird part here, the UUID in $route_parameters in the first run does not match the UUID loaded in the pane, the second time it does match, so this first run never shows the correct preview node.

So knowing this I went to check the PreviewPaneController.php, so the controller for the pane. An interesting thing happens here if I put a breakpoint, at any point, in the view method. It delays how long the pane needs to open, and this causes this second hook run to not update the preview.

So what I'm assuming happens, after clearing the twig cache, the time to build the preview page is longer and this second call happens before the pane preview page is displayed. However, when the node cache is built, the pane display is fast enough for the second call to refresh the page.

Yes of course. Honestly testing as much as possible would be very useful, there might be edge cases I didn't think of when developing.

I've implemented saving a URL to the image page, on the media type image. Using Field Mapping in the media type config, you can pick a field (at the moment I limited it to field type link) to which it will save the URL to the original image page.

So steps to use:

1. Go to /admin/structure/media
2. Manage fields for Image
3. Create a new field of type Link
4. On the Image edit page, under Field Mapping > Pixabay URL select the newly created field
5. Any newly added image from Pixabay will automatically set the URL and Original image as title

This way you can also modify the field display, however you want to, using Manage Display.

Looking at the Pixabay API, they don't return the license, however, we get the original post URL. So we can save that information.
When it comes to the license, all Pixebay content is under their Content License. So you can use that URL on your page if you want to display the license or even their License Summary.

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

Implemented sorting. Also had to implement many more changes, than what Search API Location expects, but sorting should now work. Remember how meilisearch implements geo sorting, it's probably going to be the last factor when it's collecting relevant data.

Noticed, that there is a way to sort by geo data. Looking into implementing that too.

First, to answer the question, no the current version of the module does not support spatial search.

So I started working on a submodule, to integrate with the Search API Location (FYI the current view integration in that module doesn't work correctly with Drupal 10.1, you need to use the dev version, but it will probably be fixed soon).

To integrate that functionality, I had to add some events to the Search API Meilisearch backend since I had to modify indexing, searching, and filtering. Still, I don't think the location functionality should be part of the parent module since not everyone will need Search API Location in the first place.

A few things, Meilisearch expects geosearch to be saved in a field called _geo formatted as JSON containing lat & lng variables. One way this could have been solved was to force the machine name to always be _geo, but this didn't feel right to me, so now I do these translations on the fly.
The second thing and this is also the reason I can do the previous thing. Meilisearch expects only one spatial field, so I also validate that people can't add multiple spatial fields per index.

While I think the functionality is finished, you can index location data, it correctly returns it, and filtering with it also works. The tests for new functionality still have to be added, so marking it as needs work. Also, the tests are failing because of 📌 Fix gitlab-ci Needs review

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

As I suspected replacing dependencies with needs was enough to fix it. Needs review.

admirlju → created an issue.

Implemented the custom client adapter. Something is wrong with the YAML file for the GitLab pipeline, so tests aren't running at all.

I like the idea of just using a custom Client adapter and removing it once most people switch to newer Drupal. I'll look into it.

Okay, so this is weird. After the first time, it works correctly. Narrowed it down to being connected to the twig cache in some way, every time I flush it, the problem shows up.

So the fix doesn't work if I do this:

1. Add new content
2. In the iframe settings, disable Open preview by default
3. Close and reload the page
4. Type anything in the title and click the preview button

The page is empty.

Thanks for the answer.

Noticed that nested editor elements didn't trigger auto-reload when you clicked outside the editor. Only when you click inside it. For example on Full HTML, if you add a table, fill in the content, and then click anywhere but the editor the preview doesn't update.

Added so it also listens to OnBlur for .ck-editor__nested-editable

When you say, that the preview should update on textarea. Should it be constantly changing when you are typing inside a CKEditor5? Because with the current committed code reload only happens when CKEditor loses focus.

When using select, it updates immediately when selecting a different option, that part works fine. Since I'm not sure about how CKEditor5 interaction should update the preview, I'm leaving it as needs review for now.

There is a workaround, but it's not the best. This should really get implemented.

I'll explain the workaround, but the problem is if you have a lot of menus and submenus with special characters it's a lot of work.

You have Paths to replace with custom breadcrumbs configuration, and can use it like this:
Example for node path (/articles/random/ (where I'll add a special character in random):
regex!/articles/random/\d :: Article | /articles :: Ran'dom | /articles/random :: <title>
Example for view/page path (/articles/random)
/articles/random :: Article | /articles :: Ran'dom

For your French example, it would probably look something like this, you'd need to have 3 entries and this is why it's annoying:
I'm demonstrating here if the content is represented with ID in the URL (if you use a string you can replace \d with .+):
/l-association :: L'association // Home > L'association
/l-association/presentation :: L'association | /l-association :: Présentation // Home > L'association > Présentation
regex!/l-association/presentation/\d :: L'association | /l-association :: Présentation | /l-association/presentation :: <title> // Home > L'association > Présentation > Content title

The last entry needs to use regex because it captures every possible combination. The <title> tag will use the content title, so it should correctly use special characters.

To understand better, the first part (up to the first :: separator) is pattern matching, this can either be string matching or regex matching.
From this point on you are building a custom breadcrumb, :: separator is used to separate text and URL to use on that text, | separates different crumbs. You can leave out the URL as I do with the last element. So: <first text> :: <first url> | <second text> :: <second url> | <n-th text>

Changed it so now it compares to $check_path and not $target_segment, also changed the description so it's visible that forward slashes have to be escaped with a backslash. Needs review.

I've rebased the merge request. So I can try and implement the fixes provided in #16 with all the latest code in 2.x-dev. I'm getting a similar error as @bhouge in #20. This comes down because this takes the whole line a preg_match (in my example node/3), and the problem here is because / is unescaped.

That said even when I escape it as @supriya1992 did, it doesn't correctly match when I go to /node/3 page, because we are trying to compare the whole string to the last path element (In my case were trying to compare node\/3 to 3).

Also, her config should not work in the first place report/2[0-9][0-9][0-9] should only validate to strings starting with report/2 and ending with 3 characters inclusive between 0 and 9, so a valid match would be report/2123

Testing with Drupal 10.1 and PHP 8.1.

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

I've done some testing, both with what was asked in #3 and some other options. From my tests, this works perfectly, and it's a really useful feature. I think this can be merged. RTBC.

Applied the #3171741 patch and pushed it to the issue fork. So this old fix gets merged into the dev branch.

@floown could you please provide a bit more information? With steps to reproduce your issue.

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

Meilisearch search engine is much simpler than Solr, so it doesn't have a similar concept of cores from Solr.

Meilisearch configurations are usually per the meilisearch server. So you'd have to start a new meilisearch server for each environment, especially if you have clashing index names.

If you want to migrate data from one server to another, you can use snapshots or dumps in this case dumps are probably the better choice. But this doesn't only create a copy of settings but also indexes and documents in those indexes.

But as I said, Meilisearch is purposefully made to be simple, so it can be easy & quick to set up and use.

Now how to correctly handle between different deployments of a website. Well for the most part you can export Drupal configs and import them and then by hand just replace the Server URL and Master Key, after doing that, the backend configures Meilisearch.

So I was thinking, since Drupal 9 is EOL, should this patch change code style, to use php 8.0+ syntax?

With this change phpunit for Drupal 9 is expected to fail. This should probably be a separate issue, probably related to dropping Drupal 9 support/new major version.

Opening this issue up, now that Drupal 9 is officially EOL.

To make it easier for maintainers create a merge request. Applied the #4 patch and tested it. From my tests, it correctly opens the dialog when a user tries to close the page.

I think this should eventually be reworked to not use jQuery as that will (hopefully) one day be deprecated. But the patch fixes this particular issue, so RTBC.

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

So I have no idea why phpstan doesn't ignore those 3 errors even when rules are directly copied from the generated phpstan baseline file.

admirlju → created an issue.

I've created an issue fork and tried to apply the patch to it, but it couldn't apply, so I manually added your code there. I also changed the code a bit, so Automated Logout will still work for anyone not using the Persistent Login module. It still needs review.

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

The automated tests are passing and the sanity check works correctly. I think this can be merged. RTBC.

Quickly generate a patch file, from your repository. Since automated testing on the GitLab pipeline at the moment doesn't work correctly for Search API.

So I don't know if one fix brought this problem or if it's because I'm now testing on 10.5.x, but BusinessRulesProcessorTest.php is failing. Because of EventDispatcher mock, returning null instead of object.

To me, this looks like an old core problem ✨ When trying to create a table that already exists but is empty, recreate the table rather than throwing a DatabaseSchemaObjectExistsException Needs work . Since you potentially installed the module long ago, there might be a schema associated with the search API, that is causing this problem. You could try to further debug this by using Devel module → . Hopefully, with these tools, you might be able to find the root of the problem.

Do you see Search API when running drush pm-list --no-core --type=Module? Are there any more errors in watchdog drush watchdog:show and then drush watchdog:show-one <id>

Interesting ran the tests locally just to see what is depricated. And I only get one Javascript Depreciation error for AjaxTest::testAjax. This was tested on 9.5.11.

That said it would be amazing if this gets merged and fully set up soon.

This is a good solution. But I think we should first merge one other issue, before applying this patch. Specifically talking about ✨ Use Drupal.dialog call instead of jQuery dialog RTBC in that issue @DeaOm didn't just replace the jQuery dialog with Drupal dialog but also refactored the code to use vanilla JS, it would be good to fully move away from jQuery.

And when that is merged refactor your code to also use vanilla JS. And yeah I agree about adding more tests. So for now I'll switch this to needs work.

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

Tested the MR!36 the block search now works correctly. RTBC.

To make it easier for maintainers, created an issue fork, applied the patch, and fixed some code style problems. From my testing, this does fix the problem, but leaving it as Needs review, so others also test the MR fork.

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

The patch made the module installable on Drupal 10, but there was a bug where configurations were loaded by a hard-coded block machine name (id), so I fixed this. So leaving this as Needs review.

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

The CS fixes didn't break any functionality. So this can be merged. RTBC.

I can't replicate this error. But that said I still think this patch should be merged, just so the service formatting is consistent throughout the whole YAML file. I'm leaving this as needs review, but there is no harm in merging this patch.

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

The changes worked for me. I'm just wondering if maybe there should be some tests made for this code because it's not used by any other code and someone might potentially delete it in the future. So I'll leave it as Needs Review for now, just so people can debate this a bit.

Tested on D10.1.14, it install and from quick testing by hand it also works. Ran the tests locally, and they are also passing. So I'm marking this as RTBC.

Nice and simple solution. Works as expected, I think this can be merged. RTBC.

Now we create the fields when calling the getBackendDefiniedFields method. So the error above does not show up and the already existing tests pass. Needs review.

I have an idea why this happens. It comes down to getBackendDefinedFields method in the backend. This is used to expose fixed fields to views, in our case these special fields aren't fixed so they don't have path mapping, and none of them should be available to the view.

I'd personally go with the simplest solution and just return an empty array. Since really these fields can be accessed using other fields. Or we can add fixed fields that map to these. I personally don't see the point in this second solution, but opinions are welcome.

So I've quickly tested random Japanese text in an article content type and the language left to English. I'm not getting that error on 2.x, so either it's fixed in 2.x, or potentially something else is causing it.

admirlju → created an issue.

The provided patch fixes the deprecation issue, so this can probably be merged. So now with the meilisearch, this error pops up since the special fields are indexed there, so I guess the added debugging code also works:

Drupal\search_api\SearchApiException while adding Views handlers for field on index Content index: Field 'search_api_language' on index 'Content index' has no property path set. in Drupal\search_api\Item\Field->getDataDefinition() (line 479 of /var/www/html/web/modules/custom/search_api/src/Item/Field.php).

I will be opening an issue on the Search API Meilisearch module about this, but I'm not sure if is it ok to index these fields or not. If it's ok what would be the correct approach?

That said I'm setting this issue as RTBC.

I think this needs to be postponed for a bit at least until ✨ Write the visit in Kernel TERMINATE event Needs review gets merged. At the moment implementing this with hooks would require views to not be cached and that just doesn't sound like a good idea, but as soon as that is merged this functionality can be finished.

It looks good to me. This way people can disable/increase the limit if they want to. Didn't break any other functionality in my testing so it can probably be merged.

Since it's a new functionality, decided to add an automated test for it. The new test is passing, the old one is still failing for the same reason as mentioned before.

@hktang How did it go with the 2.x version? It would also be useful to know a few more information. Did this happen on a fresh install? If not, from what to what version did you upgrade? What version of Meilisearch are you using?

@detroz that patch causes the same error. The problem here is because string_long doesn't support text_default formatter and because a formatter was used instead of a widget. Adding a new patch.

Added a simple config, to set the max length and it defaults to 128. An empty value means no limit.

As mentioned in the patch comment, the reason behind 128 is that in the core that is the default max length for textfields. However, I did find this issue 📌 Increase or remove default textfield #maxlength=128 Needs review where they are already planning to either remove the maxlimit or increase it to 256.

When it comes to this module, I personally also don't think it's smart to just remove the limit at least not by default, as @drunken monkey already also pointed out, it does work as a guard against attackers. I would either just increase the limit to something like 256 or at max 512, or make it configurable (where the default value is still 128).

Created a merge request to use GitlabCI testing. Also changed that null safe operator, since the module, for now, supports Drupal 9 even if it's going to be EOL in less than a month, and since it can technically run on php7.4 it's better to program with those limitations in mind.

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

Since this happens on generic error code 0. I've made the error message even more generic. Now when something weird with the code happens, it will say Search API Meilisearch error(s) occurred. Please check the logs for more information. more details are always logged in dblogs.

This will also have to be merged with 2.x.

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

So the NULL check is not a good solution, if you just do that you'll still get this in the dblog:
Drupal\search_api\SearchApiException while adding Views handlers for field on index Content index dev: Could not retrieve data definition for field '' on index 'Content index dev'. in Drupal\search_api\Item\Field->getDataDefinition() (line 482 of /var/www/html/web/modules/contrib/search_api/src/Item/Field.php).

While it fixes the depreciation error, you are still getting a SearchApiException that is correctly handled and logged.

Here I've made a small change in the search_api.views.inc file at least for my case it fixes it, could someone please test the patch file? Thanks.

@sarwan_verma So for some reason the tests are failing, but that said the specific fix meant for this issue is the same as the original patch file, and that does work. But the maintainer wants to fix this problem at the root cause, method docs requires that parameter to already be a string and not NULL.

So the Could not retrieve data definition error is potentially caused by the Search API module itself 🐛 PHP 8.1 Deprecated Function Needs work .