🇬🇧United Kingdom @scott_euser

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

For those stumbling across, for now we just did a module implements alter and unset simple oauth implementation to roll our own logic matching that of the project

if ($hook === 'entity_update') {
  unset($implementations['simple_oauth']);
}

Okay I think its there now matching your comments from #15 + the issue summary.
@astonvictor maybe you can check this still allows you to solve the original goal and confirm? Thanks!

The merge request is for the ongoing development, the advice in the documentation is to either download the patch or use composer patches 2. Patches make it harder for maintainers to know what to review + worst case even diverge from the merge request causing people to waste efforts. https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →

Thanks!

Okay updated the issue summary, maybe we start with the state change. So:

• dispatch TfaEnabledEvent when switch from 0 to 1
• dispatch TfaDisabledEvent when switch from 1 to 0

Then e.g. specific plugin changes if someone needs can be in a follow-up to keep this smaller.

Ah I see makes sense; is the general setup here for events + event names okay before we rework? I used the setup from https://www.drupal.org/project/social_auth → as a guide

scott_euser → changed the visibility of the branch 3532729-provide-an-alter to hidden.

@cmlara I think that makes sense thanks! And @astonvictor I believe that should work for us too right? Ie, we do not need to change the data we just need to know it has changed. I updated the issue summary + suggested we take the same data + 'original' that entity update hooks do for convenience as I can imagine responding to changes to plugins but ignoring changes to skipped validation count.

This should do it, check_markup applied (also to the value in case). The test coverage updates are mostly because that switches <b> tags to <strong> tags, but also covers stripping out an unordered list (which is not allowed in the 'footnote' text format) so that should protect against any future regression there.

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

Thank you! Manually tested as well and works well. Will get this and the other in asap and make a release.

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

Thanks all for the work on this! @bojan_dev Any preference for type of test/where? I believe just test coverage that saving AccountInterface or ConsumerInterface triggers the token deletion right, but the idea that a service can be overridden would not need testing, is that your expectation?

Moving back to needs work per #111

Okay placeholders + field descriptions updated.

Still in 'Needs work' as per the discussion about update hooks.

Great thanks for the details!

Trying to understand how this works, in the deprecated callback its clear

On load of Klaro, we can take one action or another based on consent.

But in the new UI proposed by this MR, how would that work. We have these scenarios right?

1. onInit when the user has already granted consent
2. onInit when the user has not yet granted consent
3. onAccept when the user consents during the current page view
4. onDecline when the user declines consent during the current page view

Is that right? If so, then how in onInit would we make the correct JS call based on already having accepted that service or not? The old callback form makes that distinction clear with the placeholder + description. Or maybe I misunderstand?

Thanks!

scott_euser → created an issue.

Thanks for getting to the bottom of it. So reverting this one? 📌 Remove the canonical route of the site setting entity type Active . It was the solution to a different issue though with the old flattened site settings loader, so would need to understand how to get a fix that covers both

This may have led to a small regression but I have not been able to test and confirm yet https://www.drupal.org/project/site_settings/issues/3530506#comment-1615... 🐛 Mismatched entity and/or field definitions Active

Great thank you!

Branching strategy sounds good. And thanks for reviewing!

Thanks for the tip Ruslan, was exactly that!

• Going to mark as major as there is no valid workaround except either locking to old version of core or disabling the module
• Added more detail to the issue summary + remaining issues (ie, deciding on whether a 2.x release of this module is needed - I expect that <=10.4 and <=11.1 both get broken by this, so 1.x may need to be locked to older Drupal Core and 2.x newer Drupal Core
• Updated title to help people find this a bit easier as I am not sure people will know the root cause is the Drupal Core underlying CKE version update

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

FYI here is another issue that results in plugin not found in case anyone stumbles across this: 📌 Support CKEditor5 45.x Active

This issue is probably better as closed as it seems unrelated to this module. The comments here pointing people to potential culprits for the unclear error is likely sufficient. Feel free to disagree of course

Sorry for the commit guessing! Running a big migration script locally, didn't want to interrupt it with ddev router. This is now ready for review + has test coverage.

If only a single bundle is selected, the widget reverts to the default Core autocomplete. In addition to responding to any sort of alter, this has the additional benefit of making it easier to convert an existing site because autocomplete filters can just default to this widget with a sort of find and replace in form display config.

scott_euser → created an issue.

Thanks for all the work on this module. Still learning how it works, but I made some suggestions on how it could be approached. If this is something that could be considered and if I could get a general steer I could work on this myself. Thanks!

scott_euser → created an issue.

Needs work to get this updated for 5.x

scott_euser → created an issue.

Thanks for the work on this! Added a comment on the MR. Needs a retest, but then sounds okay to me. Thanks!

Was simpler than I expected, seems its a true drop-in replacement element for standard checkboxes element:

Thanks for testing, I need to do another test run myself, there has been plenty of Clipboard Pipeline activity lately in CKE https://github.com/ckeditor/ckeditor5/blob/stable/CHANGELOG.md - perhaps they have fixed the issue via some tother route. Ideally we also get more test coverage to ensure the pipeline continues to be modify-able but need to consider a bit more... my attention has been elsewhere, thanks for you patience.

I think you're right but may be a little bit because I want to properly retest it then. My route to booting up Word as a Linux user is firing up a very old slow laptop so a bit more involved than my normal testing :) Thanks for helping test and push things forward on these issues.

scott_euser → created an issue.

Hi! This module just opts in to using the field, it does not actually provide the functionality. You would needs to raise an issue with Drupal Core (or move this one). Thanks!

Basically previously the clipboard content was pasted into the editor and so any other modules or tools wanting to modify the clipboard content would not be able to.

So instead the different CKE plugins are meant to modify the clipboard contents and then let it continue on until eventually CKE Core pastes it in.

They call this the clipboard pipeline. By it's similar to drupal alter hook: run an alter and let as many modules as desired alter one after the other. Then finally continue on with the original action.

The problem though is there are two clipboard pipelines rather than one it seems which is why that upstream issue in CKE mentioned in #13 is blocking this.

scott_euser → created an issue.

Please give a thumbs up at https://github.com/ckeditor/ckeditor5/issues/17309 to upvote the feature in Cke5 blocking solving pasting conversion.

Please give a thumbs up at https://github.com/ckeditor/ckeditor5/issues/17309 to upvote the feature in Cke5 blocking solving pasting conversion.

Great thanks, I'll try to get this back to you in the next several days. Thanks!

Yep makes sense to keep that separate
Thanks!

Hi there!

I'm not sure though where this can be addressed - I'm thinking maybe further up the inheritance chain in EntityReference class?

Correct yeah, all this module does is alter views data to make use of the Entity Reference filter in core (ie, opts in to it https://git.drupalcode.org/project/views_core_entity_reference/-/blob/1....). It does not actually provide the entity reference filter. So the issue would need to be raised in Drupal Core itself if you have found a bug with it.

As a workaround you can specifically only opt some fields into the new filter per the "Alternatives to using this module" instructions on the module homepage https://www.drupal.org/project/views_core_entity_reference →

Hope that helps! I'll move it to works as designed here, but rather than create a separate issue you could also decide to move the issue 'Project' entirely over to core and re-open.

Sure done

Adding credit to @andreastkdf for spotting this

scott_euser → created an issue.

Haven't heard anything further nor further reports, so going to assume this was resolved on the project end

Given we are now moved on to 1.1.0 anyways, if someone can have steps to reproduce we can reopen

Great thanks for confirming!

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

Thanks, sorry for being a bit slow addressing feedback! Happy to handle the mix of combined inherited + permission.

Wondering if the UI should become a tableselect like this instead as some of these descriptions are getting rather long. Perhaps this makes it easier to read? I left both on the screen for now to get your preference.

Nice work, thank you!

Okay I believe this is now what you described, I moved the choice of action to before the push params event, then pass it as a new argument with set/get methods. Only odd thing is that push params event is also called in PullBase hence the default value being needed here as changing the action is only appropriate to the MappedObject context.

Sure sounds good, thanks for the quick review!

I suppose its fine too; I searched all code that calls querySearch and they all seem to pass a limit so it probably has no other consequences.

I don't understand why the 16,384 limit passed already is not fine though as that seems to match Milvus limit documentation: https://milvus.io/docs/limitations.md - is it indicating that this is a symptom of a wider problem of us not handling limit/topk correctly in Milvus?

Added test coverage for the event as well now, ready for review.

scott_euser → created an issue.

Just hiding patch to avoid confusion, the recommended way now in Drupal docs is downloading locally from the diff and placing eg in private patches folder within your repo

Note that the UI is completely revamped and more than what is highlighted in the issue summary has changed; namely:

1. The creation of a project process is different
2. The setup of a Client is different
3. Obtaining the Secret is no longer a one-time thing and happens at a different step

I added details for adding multiple Redirect URIs as well

Here are the updated steps for the Google Console side of things (will create this as an MR to the readme as well):

1. Log in to a Google account.
2. In the project picker it will default to 'My First Project'. Click that to optionally create a new project with a more appropriate name.
3. If you created a new project, select the project you created from the welcome page https://console.cloud.google.com/welcome once it is available.
4. Under 'Quick access' click 'APIs and services'
5. Navigate to 'OAuth consent screen' and click 'Get started'.
6. Under App Information, set the App name and User support email then click 'Next'.
7. Set the Audience to 'External' then click 'Next'.
8. Set your contact information then click 'Next'
9. Read and - if you agree to the Google API services data policy terms and conditions - check the box and click 'Continue'.
10. Finalise by clicking 'Create'.
11. You are taken to the 'OAuth overview'. Click 'Create OAuth client'.
12. Select "Web application" in the Application type field.
13. Set the Name field as desired.
14. In the Authorized redirect URIs section click Add URI.
15. Paste the URL copied from Step 2 in the URI field (the URI found in the 'Authorized redirect URL' within your Admin > Configuration > Social API > (Provider) settings page). Multiple URIs can be entered if you wish to allow multiple environments to use this Social Authentication.
16. Click Create.
17. Copy the new 'Client ID' save it somewhere safe.
18. Click on the newly created Client from the Clients list to find the 'Client secret' and optionally manage secrets. From here you can also add additional 'Authorised redirect URIs' for additional environments.
19. Note that it may take up to 5 minutes for changes to take effect.

It's here that work needs to be done and more info can be found https://www.drupal.org/project/footnotes/issues/3486079 🐛 Pasting should update the clipboard content rather than inserting Active

Thanks!

No new phpcs/cspell, the warnings are the same as being addressed 📌 Fix phpcs pipeline Active

scott_euser → created an issue.

Before:

After:

scott_euser → created an issue.

Came across this as well, resolved the phpstan issue. Thanks!

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

Feel free to use patch for now, I don't want to make a release for every commit + important to let more people encounter this in case of regression not covered by test coverage. One of many projects I maintain. Please leave status as Fixed as release process is unrelated.

Added some further test coverage in, merged, thanks all!

Sure, happy to see something like this get in. Perhaps in Drupal\simple_cron\Plugin\SimpleCron\Queue::getTypeDefinitions() to avoid breaking change (haven't checked, but guessing there would be one), could check if the definition exists and if so append some sort of '_' . $count to it so e.g. `eca`, `eca_2`, `eca_3`...

I think this needs an update pulled from 1.2.x as the test coverage is failing. It should also be somewhat straightforward to add test coverage for this into AiSearchSetupMySqlTest

Thanks!

Can you put it in a merge request so it can be tested/reviewed?

I think this is now a duplicate of the work that is mostly done in 🐛 Fix failing pipelines Active