🇬🇧United Kingdom @scott_euser

Thanks for the review! Needs work actually as ajax loaded inputs aren't saving, I clearly didn't test my own work well enough :)

The problem here is not a silently broken site, it is a broken site that you cannot repair, because even in maintenance mode drush cr always fails because of the locks put on cache_config by search_api_views_data triggered by visitors requesting pages. The more visitors we have, the more risks we have of never being able to complete a drush cr on a production site.

Yes, we have had to work around exactly this in high traffic sites that silently failed, temporarily deleting the view and recreating it, e.g.

drush config:delete views.view.example_search_api_view -y
drush cache:rebuild
drush config:import -y
drush cache:rebuild

Which I guess is triggering the views_data rebuild

As a temporary code to discover and self-repair the issue via a cron job we have run this for our specific in case its helpful for others:

  /** @var \Drupal\Component\Plugin\PluginManagerInterface $views_row_manager */
  $views_row_manager = \Drupal::service('plugin.manager.views.row');
  $node_row_definition = $views_row_manager->getDefinition('entity:node', FALSE);
  if (!$node_row_definition || empty($node_row_definition['entity_type'])) {
      /** @var \Drupal\Core\Plugin\CachedDiscoveryClearerInterface $plugin_cache_clearer */
      $plugin_cache_clearer = \Drupal::service('plugin.cache_clearer');
      $plugin_cache_clearer->clearCachedDefinitions();
      /** @var \Drupal\Core\Cache\CacheTagsInvalidatorInterface $cache_tags_invalidator */
      $cache_tags_invalidator = \Drupal::service('cache_tags.invalidator');
      $cache_tags_invalidator->invalidateTags(['views_data']);
      /** @var \Drupal\Core\Cache\CacheBackendInterface $discovery_cache */
      $discovery_cache = \Drupal::service('cache.discovery');
      $discovery_cache->deleteAll();
  }

I am sure this was overkill, but at least gives a new chance for the issue to get fixed when it silently fails, in this case for a Search API View of Nodes.

In terms of moving this forward, would the 'second try' + log instead of exception MR be at least reducing the risk of silent failure for now? Happy to work on this if anyone can point me in the direction of how we might lock the config as outlined in #46.

I think bulk do all could be potential follow-up for those who trust it to get it right without real review process

Here we go:

No activity, unassigning, will push MR for this

Thanks! I think 1.2.0 safer, gives a chance for more testing with a wider group. Added note on how to test to the issue summary.

I think it's this one in AI module you're after https://www.drupal.org/project/ai/issues/3513409 🐛 Warning: Undefined array key "#entity" in Drupal\ai_translate\Form\AiTranslateForm Active Please give that a shot and reopen if not. Thanks!

Hi! Thanks for the contribution, can you make a merge request instead so tests run + easier for us maintaining to review/comment. Thank you!

Aha I see, this is a problem in Milvus - still rather than moving I opened a new one ( 🐛 'Special Fields' created by Search API for Views are not actually fields and cannot be applied as Milvus conditions Active ) as I expect until releases are made its likely people will stumble across this. Will update the issue title as well to make it more findable from the error.

Thanks for digging and trying to figure it out!

scott_euser → created an issue.

how would it be retrieved in any other method of the class if needed?

My point isn't that its not necessarily not needed, but that if it is needed elsewhere (is it??), adding a side effect to getClient() does not seem like the right way to do it, so more that this approach is temporary code to solve specific problem in 1.0.x. Have not looked deeply, but maybe some way in AiVdbProviderPluginManager::createInstance() to construct the provider e.g. in AiVdbProviderClientBase...

Makes sense thank you!

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

Looks good thanks! Just some minor phpcs issue flagged

scott_euser → created an issue.

Yes meant to work, probably an oversight when Group functionality was merged (was a very long standing issue with many contributors over years) https://www.drupal.org/project/footnotes/issues/3098138 📌 All footnotes available as block for Layout Builder / Paragraph / ... Fixed though I haven't revised that deeply to check.

So each footnote text has a hash of the text in the ID. The ID is unique but when stripped to get just the hash bit, if the hash matches, then the citation should target the same reference as already stored for output. If the hash doesn't exist the citation should cause a new reference to be created.

When "Collapse footnotes" is selected do we expect to see our content annotated with [1], [1], [2] (since we have 2 instances of footnote 1)? Or with [1], [2], [3] where [1] and [2] both link to a single line in the footnotes block that reads something like [1],[2]: "A"?

[1], [1], [2] in that case and that's what FootnotesFilterPluginTest::testCollapseFootnotes() covers. Group is essentially an extension of that to handle multiple formatted texts on the same page (and the JS version, multiple on the same page with lazy loading/ajax loaded content). The thing not part of offline medium is the backlinks since linking back to source references is not a thing offline.

Ultimately the module has to take some opinionated decisions, and if people aren't happy with those opinions, needs to be flexible enough to allow control - but that of course adds complexity, hence the push always for more test coverage or it will end up being impossible for me to merge things confidently, so that was one of the bigger chunks of work when building out the current version. Some footnotes style guides for print media will say [1], [2], [3] and converting [2] to use `Ibid` when its a repeat of [1] - something I can't imagine being easy with the reference being formatted text, but anyways if someone wants to contribute that it'd be a separate feature needing its own test coverage.

Yes CKE5 clipboard pipeline has a known issue in upstream which occasionally causes test failures (they essentially have 2 pipelines, they have a PR open to streamline into one). Retrying test passed.

I also added some basic test coverage for this (though didn't go as far as extending search api's test processor base...) 📌 Add test coverage for search api ignore citations processor Active

scott_euser → created an issue.

Thanks!

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

Thanks both for figuring out the problem and working out solution! Looks sensible

We should also add to the tests so it stays fixed. Ie, adding an example of two identical texts getting collapsed & not collapsed to FootnotesGroupBlockTest for example. FootnotesFilterPluginTest::testCollapseFootnotes() already has this outside of the block context, so the issue here is no coverage for within the Group block.

scott_euser → created an issue.

Yeah I don't have access, good to know its Kevin, will ask him for it then, thanks!

scott_euser → created an issue.

Updating title since its no longer really exploring and is now merge ready (IMO)

All updated/resolved, thanks! Also added further test coverage.

scott_euser → changed the visibility of the branch 1.1.x to hidden.

Okay cool, let's leave it active then. Not sure we'll get any more drupal 11 fixes given we've got it working in Drupal 11 and there is a stable release for that and gitlab CI monthly pipeline should also notify on pipeline issues, but I'll set a reminder for summer 2026 D12 release and then we'll at least know for certain for the future.

Added some basic test coverage

Thanks! Yep, I think that code is more Drupal as a whole and loading entities rather than something for this module

Yep still an issue in 4.x unfortunately. MR still solves.

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

Makes sense and thank you for the detailed explanation. I added a documentation page nested within configuration with options + tried to capture your recommendation with examples.

scott_euser → changed the visibility of the branch 3521300-document-tfa-and-rest to hidden.

Thanks, looks good!

The reason the behaviour exists for controlling/overriding it centrally is that sites can have many many site setting types, using manage form display for each one is time consuming via the UI (yes I realise you could bulk do via config/sync/ but I think its fair to say the average site builder won't know how to do that)

Can you provide more info? Maybe the UI needs to be made more clear that you do the configuration in Pinecone and just select the one you created within Drupal

Hmmm not sure that's the right approach because we should be granting credit for the work done. I instead added a scheduled monthly pipeline to Gitlab CI in case there is a deprecation added to D10 that is removed in D11 (though I think that's not possible now and only deprecations would happen in D11 for D12 which would get created as a new issue by the bot?). The scheduled pipeline will also flag deprecations added to D11 that will affect D12.

I'll switch back to fixed for now, but feel free to disagree of course.

Looks good!

The non-JS option will always struggle in some scenarios because the only way a layout builder/experience builder/paragraphs based content can build up the combined footnotes as is is when they get rendered (or at least currently). You may get more mileage with the JS based grouping instead as the numbering, etc is all calculated after the page fully renders including getting recalculated if new elements arrive on the page (e.g. via bigpipe or via ajax).

But yeah if you did ever want something to get in:

1. Needs to be via merge request so coding standards and test coverage gets run
2. Needs to have test coverage run for the change
3. If the change could potentially negatively affect existing sites, needs to have an opt-in feature

Added a small comment, unlikely scenario, but better to be safe

Added some comments to the MR. I still think (per slack) that this should never go to 1.1.x though as getClient() should just get client and not cause side effects. And since 1.1.x has a mechanism to get the server, this is really just temporary throw away code for 1.0.x

Thanks Marcus!

scott_euser → created an issue.

Thanks for the update. Just a bit of admin for now, changed target to 2.0.x branch and hid patches in favour of MR

Thanks for the work on this. Target should be 2.0.x branch with merge request instead please. Thanks!

Thanks for the work on this. Target should be 2.0.x branch with merge request instead please. Thanks!

Thanks! And in regards to this issue 2.0.x is now default branch

Rather than forever more patches, if next traveller here could please do a review to get this to RTBC to hopefully get it committed so we all save time in the long run :) Thank you!

Sorry for the nitpick but still introduces a couple cspell issues. Thanks!

Okay did a bit of issue clean-up. Now targets 2.0.x as expected, hid extra patches and branches to avoid confusion. Per https://www.drupal.org/node/3376090 → this is now critical as the core functionality of this module (to merge terms) is not usable in Drupal 11 which is a supported version of Drupal Core.

Would be great to get an RTBC from someone in the community so we can get this infront of maintainers eyes. Thanks

scott_euser → changed the visibility of the branch 8.x-1.x to hidden.

Change record causing this fatal error: [#3376090]

scott_euser → changed the visibility of the branch 3499000-typeerror to hidden.

When the config is correct BUT it is overridden, this alternative message is shown. Without the MR, the link takes the user to the configuration page of Symfony Mailer only to find Brevo is already set as the default.

scott_euser → created an issue.

Makes sense! Gave the last one in 'must' a good test run before & after and commented there.

Thanks for progressing this! I tested it out by:

Without MR:

1. TOTP + Recover code
2. Attempt to login
3. Click use recovery code to get to that form
4. Disable Recovery code as option via admin account in another browser
5. Submit recover code
6. Successfully login

With MR:

1. TOTP + Recover code
2. Attempt to login
3. Click use recovery code to get to that form
4. Disable Recovery code as option via admin account in another browser
5. Submit recover code
6. Fail to login

Small change, but will at least be helpful for future me and perhaps other devs

phpstan issue unrelated and occurs on 2.x

scott_euser → created an issue.

scott_euser → created an issue.

Removing duplicate 🐛 Resolve SA-CONTRIB-2024-003 in 2.x branch Postponed (its now just once). Thanks for all the great work on this!

Yep!

Okay have had a proper think about this (apologies for the delay, holidays!). I think its a great approach as it should make the memory usage handling by Core instead of us since anyways core has to efficiently gather a huge amount of views data. Extensibility by other modules for custom config views data also becomes easier for them which may help make this into more of a framework module that others leverage.

I think in order to not break things we probably first need to:

1. Have test coverage of existing available views data (kernel sort of tests I think rather than just the functional we have) so we can compare before and after available data
2. Separate the views data (the core functionality) from the default views installation, making those 'opt-in' instead of install all 📌 New administrative page to opt-in configuration entities for views support Active

I'm happy to work on both of those first over the next months.

Thanks! This is working well, I was able to complete the task without issue and able to index huge content items; used extensively long wikipedia articles on old HMS/RMS ships. So I think fundamentals are good and we are getting into the finer details now. Nice work!

scott_euser → changed the visibility of the branch 1.0.x to hidden.

scott_euser → changed the visibility of the branch 3487487-improve-ai-search-datasource to hidden.

Added test coverage as well. Existing phpstan/d12 depreciations should remain out of scope.

Would be great if someone can review so we can get this fairly long standing one in.

Thanks!

Nice one, thanks Riya! This will be part of next release, so you can continue to use the patch from the MR diff for now.

Okay sorted for 2.1.x. Tests pass again BUT this could use more test coverage for the new functionality.

I also implemented a variation of @jrockowitz suggestion to improve the UX with checkboxes rather than select multiple.

For those updating to this patch, since update hooks are not done between patch versions, the simplest is to:

1. Control + F your config/sync/ directory for 'editable_tags'
2. Nest them within the tag group ID like this diff below:

 settings:
   editable_tags:
-    title: title
-    description: description
+    basic:
+      title: title
+      description: description

scott_euser → changed the visibility of the branch 3108108-allow-which-metatags to hidden.

scott_euser → changed the visibility of the branch 2.1.x to hidden.

Hmmm can't seem to get the fork to sync from upstream repo, not sure if its because I'm not the owner of the fork. No possibility to create a new fork from 2.1.x. I could create a new issue, but shame to lose all the history

scott_euser → changed the visibility of the branch 8.x-1.x to hidden.

scott_euser → changed the visibility of the branch 3108108-allow-which-metatags to active.

scott_euser → changed the visibility of the branch metatag-3108108 to hidden.

Thanks, looks good - I wonder however whether we should add this to the end of the <body> (appended inside it) in case <details> is subsequently wrapped in something else that is collapsed?

Beyond that just some coding standards clean-up flagged (see merge request > pipeline warning for eslint, in the right side when viewing the failure on desktop is a download button that will contain an _eslint.patch you can apply to your merge request).

Sure happy to help; will make sure to broaden test coverage as well to make it easier to maintain long run. As an FYI I extended the module for a particular use case via https://www.drupal.org/project/permissions_view_only → which is useful for a few of our clients who want more transparency as to how we configured Drupal without giving them access, hence my interest in Filter Perms module.

But yeah, TL;DR happy to co-maintain, thanks!

Sorry wasn't aimed at you necessarily, just the notification reminded me of this issue and wanted to comment what is needed to move forward :) Thanks for confirming RTBC in any case!

Merge commit here since drupal.org issue based merging was failing: https://git.drupalcode.org/project/search_api_rabbit_hole/-/commit/cdfe3...