Account created on 22 July 2015, almost 10 years ago
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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']);
}
🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

@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.

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

Okay placeholders + field descriptions updated.

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

🇬🇧United Kingdom scott_euser

Great thanks for the details!

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

Branching strategy sounds good. And thanks for reviewing!

🇬🇧United Kingdom scott_euser

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
🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

Yep makes sense to keep that separate
Thanks!

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

Adding credit to @andreastkdf for spotting this

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

Sure sounds good, thanks for the quick review!

🇬🇧United Kingdom scott_euser

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?

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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.
🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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`...

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

Production build 0.71.5 2024