🇬🇧United Kingdom @scott_euser

Ah and the attachment here →

Thanks for progressing this! When I tried it out I hit a fair number of errors that I had to hack around to get it working (comments added to MR). When it is chunking its still fairly confusing. In my attached video I have 3 items being indexed, all get chunked in various quantities of chunks. It seems to say 100% quite quickly but does a lot more and I had trouble following along to understand where I am in the process. It does seem to properly index it all in the end.

Confirmed tests pass locally again when applying the patch from 🐛 Cannot create Search API based views On Drupal 11 Active

Thank you for sorting this one out! This also fixes the AI Search module (sub-module of AI core) where tests had to be disabled temporarily as a result: 📌 Reinstate MySQL tests Active

Very vague error so it was a real pain to see where it was going wrong. For convenience, the error:

Ai Search Setup My Sql (Drupal\Tests\ai_search\Functional\AiSearchSetupMySql)
⚠ Field indexing options
✘ Search view
┐
├ Exception: Warning: Undefined array key "title"
├ Drupal\views\Views::fetchPluginNames()() (Line: 157)

Looks like the problem is upstream in 🐛 Cannot create Search API based views On Drupal 11 Active

Thanks both!

Closing per comment above unless we hear further.

Closing per comment above

Thanks for the contribution, however Taxonomy Index Tid is a valid filter though and already allows things like BEF to work on it (compared to numeric/string entity reference filters).

If this module was to accept such a change it would need to be opt in, not default (e.g. a new config page) and include test coverage. Otherwise we age forcing a new behaviour on all existing users of the module which meant others may not want. For now will close but if multiple people feel strongly and are willing to contribute the above, could reconsider.

Okay that now handles:

• New module install, navigation not yet enabled but gets installed later (hook_modules_installed kicks in)
• New module install, navigation already enabled (hook_install kicks in)
• Existing module update, navigation already enabled (hook_update_N kicks in)
• Existing module update, navigation not yet enabled but gets installed later (hook_modules_installed kicks in)

I think this needs a new 2.x to make this Drupal 11+ only

After:

Thank you!

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

Hi! Anything I can do here to help get this over the line? Thank you!

Yeah the problem with placing the block is we need to check if navigation is installed, but also need to react if it's not yet installed and later gets installed. I'll see if I can spend a bit more time on this to see how to do that

This commit essentially https://git.drupalcode.org/project/rebuild_cache_access/-/merge_requests... - no css needed in the end, it just takes the styling from Navigation module defaults so less maintenance burden also if they change things

Actually could just leverage the Navigation module:

Custom icon could be a potential follow-up too, but think its not worth being a blocker.

Given that Gin is now Drupal CMS + Navigation & Top bar are in place, these actions are typically more suitable now within the Navigation.

This MR creates a block but could use someone to style it to match navigation.

Steps to install:

1. Ensure dependencies Gin (contrib/Drupal CMS) & Navigation module (Core) are enabled
2. Go to Admin > Config > Navigation Blocks
3. Add 'Rebuild cache' in the add block area, untick show title, and Save

There is probably some way to install it automatically but not sure how and probably best as a follow-up.

Here is how it looks now with the MR:

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

This continues to work well for us, would be great if this one can get merged in, Thank you!

Thanks! Would you mind dealing with crediting as well pls

The CSS is a bit beyond me, but in terms of testing this, here is before & after with Environment Indicator + Devel enabled for example. Thank you for your work on this!

Before (bleeding in as issue summary describes):

After (looks great):

That one is open here in case someone is able to contribute it :) https://www.drupal.org/project/footnotes/issues/3473262 ✨ Add the ability to hide the reference text in the CK Editor content editing Active

Not a css expert but I think you can try something like scroll margin top maybe.

Glad you figured it out!

Thanks for the work on this! Tests are failing, but I can also see the problem with back button is on all steps, not just the one. Thanks!

Couldn't get it to work with Select for now without schema issues, but Autocomplete better than status quo so going to merge. Test coverage added.

scott_euser → changed the visibility of the branch 3400025-group-filter to hidden.

Per https://www.drupal.org/project/drupal/releases/10.3.0 → 10.4+ is now only supported, so we can use core views entity reference now

Thanks!

Gave this a retest

I can see with your latest #access changes, there are no additional controls or effort that existed when I first reviewed in #6, so looks good to me. Going to add a follow-up to add test coverage for translation as its not the first issue that has started as a result

Thanks both!

Leaving as needs review to get your thoughts, but RTBC+1 from me

Thanks very much for the work on this! Looks good to me! I didn't make any change other than labels/variable names, but worth a quick check if you have any major concerns @svendecabooter before I merge.

Beyond that added a very basic test coverage to this. Will add a follow-up issue to add test coverage for translation itself.

valthebald → credited scott_euser → .

scott_euser → created an issue.

Marking as postponed on 📌 Fix reliability of storing status in user data Active

First question we need to ask is do we want skipped counts being set without enabled plugins to be considered 'TFA Enabled' or 'TFA Disabled' for the backend status field as that impacts our operations choice.

Maybe that is a separate issue as a follow-up: let the site builder decide with a setting, and provide a default for new installs + update hook to maintain status quo. Feels like for the scope of this issue the status quo should remain; if skip counts don't care if TFA is enabled or not, this issue should not chain that behaviour right?

Yep you're right, phpunit 11 sorry

... With an else on the latter in case ai search is disabled I guess

LGTM thanks!

You did add a $data['backend'] == 'search_api_ai_search' when you originally added it, could this potentially be an outdated issue?

Personally I do not have any need for it, I cannot remember who/where this got raised - perhaps we postpone and they can chime in if its still required.

The idea is that AiVdbProviderInterface can be implemented in some other way than AI Search yet still be relied to actually make queries/searches e.g. for agent calls or other functionality so maybe we need something like this:

new src/Base/AiVdbProviderStandaloneClientBase.php:

abstract class AiVdbProviderStandaloneClientBase implements AiVdbProviderInterface, ContainerFactoryPluginInterface {
   ...
}

existing src/Base/AiVdbProviderClientBase.php:

if (interface_exists('\Drupal\ai_search\AiVdbProviderSearchApiInterface')) {
  abstract class AiVdbProviderClientBase extends AiVdbProviderStandaloneClientBase implements AiVdbProviderSearchApiInterface {
     ...
  }
}

Okay all green now

Sorry I know you have this assigned to you, but just taking a quick stab at it based on suspicion of problem, feel free to revert of course

Okay fixed it now; turns out install hook was outdated for vocabulary so there was incorrect default value getting set. Sorry I did revert your commit @sijumpk - required getting inherited when the element is marked as required in form API should be fine on the table because there should always be a default value. We could always deal with in a follow-up if there is a use-case where people don't want to provide a default prompt I suppose but at the moment that causes a new test failure.

So back to tests passing (beyond the warning that is across the board at the moment, which I commented at 🐛 Fix phpunit tests for Max Version Active the reason).

At the moment "phpunit (max PHP version)" fails because it attempts to install php-8.4.0.tar.xz from source yet "PHP 8.3.23" is the runtime. Max PHP version should be 8.4, not sure if its us or gitlab CI that is overriding that

Any console messages or something like that? The video looks like its extremely low pixel quality so not really possible to learn anything from it (or at least how it plays on my machine)

Fixed an issue with states not applying, but can't yet figure out what's wrong with the tests, the '$this->assertSession()->waitForText('New prompt details');' doesn't seem to be waiting for it to appear here:

In case anyone can spot the test change needed to get it to wait, otherwise will try to come back to this and get to the bottom of it

scott_euser → created an issue.

I think we should close in favour of 🐛 New user should be validated before save Needs work which is a duplicate it seems but has more engagement there. Feel free to disagree of course

Hmmm I'm not sure we should actually prevent validation. I tested this out and with required fields, its impossible to register via social auth as you hit these validation failures.

Instead without this patch we are having to do event subscriber KernelEvents::RESPONSE to check if

1. user logged in
2. route is not logout
3. and user has violations (like the MR validates)

And if so, redirect to edit profile with warning message (and continue to force it until user has satisfied the violdations

If this route was to go ahead, at least it should be opt in via a settings form.

Marking as NW for discussion.

Okay updated info yml and composer json minimum versions, thanks!

I think maybe the verdict is still out on this https://www.drupal.org/project/drupal/issues/3067979#comment-16178128 →
Need to defer to other maintainers

NB Given prefer-source is an option major doesn't fit https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... → and similarly not really a bug per criteria there either particularly given its an opinionated decision.

scott_euser → changed the visibility of the branch 3032517-views-uncontrolled-fields to hidden.

Actually this is already doable with InOperator too, reverting description, sorry for the noise!

I have tried to elaborate on this a bit more to make it more clear the use case and intent. Not sure the patch is right, as you can already set default selections in the default InOperator from core (which this extends). Really the only thing you cannot do is prevent control of particular fields (ie, where you want them required).

Aha makes sense thanks!

Okay and from there on your end you'll then create new branch as appropriate? Or do we do this in 2.1.x?

Will have a look, thanks for flagging!

Sure go for it, sounds good IMO. As long as it still fixes the issue if that's what the documentation recommends... Thanks!

Thanks for the progress here! I added feedback to ✨ Improve the AI Search recursive retrieval of a specific quantity of results Active

Thanks for all the work on this and apologies for the delay. Keen to hear other opinions as I have been struggling to focus on this lately, but my general feeling is that we are repeating a lot both in AiVdbProviderClientBase/Interface + in the SearchApiAiSearchBackend.php

Looking here and at ✨ Make use of Milvus' Grouping functionality Active it seems both exclusion and grouping are both small modifications but we are making separate methods and repeating I guess to avoid breaking change, but maybe we are adding a lot of complexity to avoid BC while we are still able to, and maybe coordinated release lesser of two evils.

Then knowing that a VDB supports Grouping only means that the SearchApiAiSearchBackend needs to skip the iteration attempts when chunks are not wanted && grouping supported. And the exclude_entity_ids just added as a filter param if supported. Not actually sure if we need the supportsNotInFiltering() because there are plenty of things not supported by filtering in VDBs and ultimately if it is supported the VDB will get through the interation quicker to get the desired number of results.

Then we should be able to avoid 3 separate doSearch methods which have a lot of repetition and instead the supportsGrouping can just be what stops iteration, and we can always attempt to apply exclusions regardless of whether iteration happens or not. So hopefully results in a lot less code change in SearchApiAiSearchBackend.

I say this all without actually properly trying it, as perhaps it has been tried and its not doable. But in any case, first step I think is agreement together how much breaking change we are okay wtih

Added details about how we might work around it in case anyone is able to pick this up.

Okay I merged the compatibility with paste filter, and moved the compatibility with Word paste to a new issue. However it is still an issue as described there.

Merged thanks

scott_euser → created an issue.

Thank you!

Okay I retested manually at least with LibreOffice and its successful. The test coverage pass (though these are only as close as I could get to a real paste scenario so are not 100% like real pasting). Still pasting from Office365 on a windows machine does not contain and footnote data. But in both the branch and in 4.0.x this is a problem so I will not let this block merging for compatibility with paste filter.

I think its the same but with less debug output, so I'll review the previous MR and hid that one. @astonvictor otherwise let me know if I missed something

scott_euser → changed the visibility of the branch 3486079-pasting_ to hidden.

Yeah that sounds like a good plan re #6 thanks!

Just marking as NW since the patch no longer applies.

scott_euser → created an issue.

Confirmed, with this MR, the first name and last name of User entity get populated. Thanks!

So we should close this one though right?

Thanks! I think still somehow its too strict though. E.g. if I run

ddev exec -d /var/www/html "./vendor/bin/phpunit -c ./core/phpunit.xml.dist ./modules/contrib/tfa/tests/src/Unit/TfaContextTest.php"

At:

/var/www/html/modules/contrib/tfa/tests/src/Unit/TfaContextTest.php:430

I get false errors like

-    'stringTranslation' => MockObject_TranslationInterface_00404523 Object #398 (
-        '__phpunit_state' => PHPUnit\Framework\MockObject\TestDoubleState Object #433 (
+    'stringTranslation' => MockObject_TranslationInterface_3106f881 Object #2004 (
+        '__phpunit_state' => PHPUnit\Framework\MockObject\TestDoubleState Object #2006 (

Which are nearly the same and for the purposes of the test should be considered the same I believe

You can see this passing combined with ✨ Provide an alter for user settings Active at https://git.drupalcode.org/issue/tfa-3532729/-/pipelines/541506 but I subsequently reverted

I'm sure its not right, but sort of proof of concept...

Bit beyond me as the code there is a little hard to follow how different scenarios get stored as user data in different forms

scott_euser → created an issue.

scott_euser → created an issue.

Okay back to needs review. Added a couple explanatory comments in case. Thanks!

Thank you!

I think first probably need more advice/opinions to help decide way forward here before putting more time into an MR

Hmmm I am not sure - is that what a user using a screen reader will want? If they come across a citation, would they want the flow of the text being read to be stopped to read out the reference? Ie, that is what this will do right? Instead of 1 2 3, it will read out the full reference.

Possibly it could be more desirable to read something like "Citation 1", "Citation 2". Whereas reading out the full reference is maybe better left to if the user decides to set specific focus on the anchor link to not interrupt the reading - then it would be aria-described-by right? Although harder to achieve here as there are multiple ways to output the references.

Makes sense; I updated the code. Rendering $value was breaking tests e.g. ::testCitationUniqueTextSameValue() but I can see its already running through Html::cleanCssIdentifier() if not is_numeric, so I believe that is safe. Did a tiny bit of scope creep to rename a variable to avoid confusion with the variable name being checked to auto-collapse multiple identical texts together.

kristen pol → credited scott_euser → .

Thanks for the feedback, fixed both. Thanks!

Hi @prudloff,

I think what that comment is suggesting though is actually what we are doing; ie, rendering for elsewhere other than the page. We do this to collapse multiple footnotes together + create the ref_id (which people could anchor link to). If we instead do not render it, we could change the behaviour of either. The code in check_markup is ultimately doing the same thing, just also triggering Renderer::renderInIsolation() to get the string version earlier on. Nervous to otherwise cause further side effects for a fairly well used functionality.

Ie, we might change the behaviour of this bit

if ($this->settings['footnotes_collapse']) {
  $key = $this->getMatchingFootnoteKey($text);
}

If we e.g. switch to comparing the text_clean version (which still then at least requires that to be rendered early).

So I think its better to merge this and if you feel strongly that we should not render as early as we are currently doing, we can carry on in a follow-up.

Thanks,
Scott