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

Merge Requests

More

Recent comments

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

Change record causing this fatal error: [#3376090]

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

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

phpstan issue unrelated and occurs on 2.x

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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.

🇬🇧United Kingdom scott_euser

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

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

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!

🇬🇧United Kingdom scott_euser

Hiding patch to avoid confusion.

I can't change to RTBC since I've been doing the work so next step is someone here in the community confirming they've reviewed and tested to change the status so we can get this back infront of the maintainers now that feedback is resolved + test coverage is in place.

🇬🇧United Kingdom scott_euser

Pretty cool! I think we need at least to say that a provider supports external data source + need to consider how other providers handle this (if any) so that we build it in a way that makes it usable by other providers if/when they start to offer it.

Then needs a related issue in https://www.drupal.org/project/ai_provider_google_vertex to opt-in to the extra configuration option.

I can see the argument for keeping it in 'Chat'.

This is going to need opinions from other maintainers though, particularly Marcus I think.

🇬🇧United Kingdom scott_euser

Thanks for the contributions!

Few comments:

  1. We should at least warn that backlinks will no longer work (e.g. 1-5 won't link back to 2, 3, etc)
  2. I believe we should use 'en dash' to match standards
  3. Is the idea of preprocess hook because we don't have a single spot in the plugins/classes to build them? Wondering if maybe it's time to have a footnotes links builder service. Worried if we put the code to build them in too many places it'd be even harder to fix issues.
  4. And finally this needs test coverage, too much going on in the footnotes backlinks already to add things into there without coverage

Thanks!

🇬🇧United Kingdom scott_euser

Is this a new feature some providers offer? Can you link to docs of such a provider to help provide context?

At the moment AI Search submodule handles the integration with Vector Databases to to RAG type stuff.

🇬🇧United Kingdom scott_euser

Ah okay, in that case of you look in the module source code you could copy how fields are rendered from the twig functions in your own code, e.g. https://git.drupalcode.org/project/site_settings/-/blob/2.0.x/src/Twig/T...

But ultimately Stie Settings are just field able entities like Nodes or Terms so getting a field value is the same, e.g. $entity->field_name->value.

🇬🇧United Kingdom scott_euser

This seems right to me; its a static variable that avoids its re-initilisation if the method is called multiple times, yet the variable is never used so it can go.

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

Before I retest would be good to:

  1. Hide the branch that isn't in use (its confusing to have 2)
  2. Fix test coverage

Thanks!

🇬🇧United Kingdom scott_euser

Looks great thank you! Only we need to target 1.1.x branch instead please, otherwise RTBC

🇬🇧United Kingdom scott_euser

Thanks for the contribution! I think maybe require-dev needs to have search_api as a dependency to avoid e.g. the phpstan errors?

🇬🇧United Kingdom scott_euser

Thanks, updated title to help others find that. I think I cannot merge this as is though without an upstream fix in CKE as I believe this fixes some scenarios but Word continues to fail.

🇬🇧United Kingdom scott_euser

I'm not sure the plan here myself, but maybe a separate issue should be raised in ai_provider_deepl. It seems like converting it to chat would lose the source/target language though so I don't think this issue should be closed. Maybe AI Translate instead needs to check for providers that provide both chat and/or translate_text, and if translate_text is found in a provider, use that, otherwise fall back to chat.

🇬🇧United Kingdom scott_euser

Okay marked as skipped for now, let's see others opinions on it, but I'm thinking not worth the energy - its the only Functional JavaScript test we have (thus far)

🇬🇧United Kingdom scott_euser

Thanks for checking! Updated with latest from 1.1.x (which introduced 🐛 Field #parents of all AI Content Suggestions are now incorrect Active ).

Otherwise this is back to ready for review. The test failure is on the simplest thing; saving of initial checkbox in form without even yet starting to test the Element, so I think its more a test runner issue, perhaps due to our custom implementation of gitlab-ci.yml to handle the AI Search with FFI extension... I think better I mark the test as skipped.

🇬🇧United Kingdom scott_euser

I think we missed a small update in the plugins after changing the form structure, created a follow-up here: 🐛 Field #parents of all AI Content Suggestions are now incorrect Active

🇬🇧United Kingdom scott_euser

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

🇬🇧United Kingdom scott_euser

In the latest version as a result of https://www.drupal.org/project/site_settings/issues/3498435 Add developer helptext (when access available) to output some site settings via twig Active (2.0.1 onwards) you should see copy-pasteable twig snippets throughout the UI to be able to output values from the full loader. These snippets match the examples on the module homepage. They are found where site settings are configured (admin structure rather than admin content area)

Still we should fix the fatal error as well though so will leave this open.

🇬🇧United Kingdom scott_euser

Added SQLite to list

Not sure about readme... More places to manage...

🇬🇧United Kingdom scott_euser

Great thank you!

🇬🇧United Kingdom scott_euser

Okay I think this is ready for review now. Follow-ups then needed for

  1. autocomplete
  2. @marcus_johansson's comment about supporting Twig
  3. @marcus_johansson's comment about making available for use in MCP

I don't know why functional JS test not running in 10.4 - runs perfectly fine in my 10.4 local, but its getting stuck at the simplest thing in gitlab CI (ie, open a form and save it, without any ajax or anything yet). Maybe someone can spot the issue with that...

To review, best to read the merge request's update to docs

🇬🇧United Kingdom scott_euser

Re #9 I have updated it so each prompt gets stored like ai.ai_prompt.my_prompt_type.my_prompt.yml so its easy to use Config Ignore like `ai.ai_prompt.my_prompt_type.*` + updated docs page to also note how Config Ignore allows you to make a specific property within a config (e.g. a Settings form in your module) be content manageable yet everything else remain as config.

Re #11 I think I don't quite know enough about that yet to know if that causes any fundamental changes to this to be considered? Any concerns Marcus, or is that more a subsequent bridge between this and MCP and not really changing the AI Prompts themselves?

🇬🇧United Kingdom scott_euser

Config entities can before more like content with Config Ignore but the other way around it's much harder to make content deployable. Same for machine names vs IDs, much harder to make deployable target to UUID from content. All prompts are currently already config (at least in AI suite of modules), so making the config target content also problematic (and we will have a lot of unhappy developers if config export from production leads to changes).

Contrib/Core have largely not had such machine name issues with vocab names or entity type clashes, and we could easily advise people to prefix with module name or abbreviation for example.

This matches for example Webform: commonly people want to have forms deployable and therefore it's config entity by default but it's clear (and even in suggested ignore) how to use Config Ignore to let site builders give content management control over all or some webforms (e.g. wildcard ignore webform.* but unignore eg !webform.specific_form to make just it deployable.

🇬🇧United Kingdom scott_euser

Hmm yeah since we don't document this in mkdocs any more, seems this is outdated but similar. List matches so I thikk we can close

🇬🇧United Kingdom scott_euser

Okay ready again, thanks for merging in the gitlab ci one!

🇬🇧United Kingdom scott_euser

It seems if you use Drupal 10 then its ?TimeInterface which is the problem. I pushed a fix that should solve and work in both D10 and D11. Please confirm.

🇬🇧United Kingdom scott_euser

Well I suppose the issue summary is suggesting to add this check just in case some other contrib modules end up doing the same and incorrectly structuring local tasks, so up to maintainers if you wish to add this to make the code more protective about its expectations of structure. In any case it solves it (or masks the problem):

  1. Enable translation eg on article
  2. Install Page to PDF module 1.0.2 or earlier
  3. Enable page to pdf for article
  4. Attempt to translate an existing article
🇬🇧United Kingdom scott_euser

Actually I think this bug is related to the code in Page to PDF module, raised an issue here that stops this fatal error 🐛 Compatibility with navigation module Active . @andreastkdf feel free to reopen if not, but I believe this should be closed here.

🇬🇧United Kingdom scott_euser

Added a section on "Multilingual content translation" based on concerns raised in https://www.reddit.com/r/drupal/comments/1jtnxuf/enabling_content_moderation_on_a_multilingual/

🇬🇧United Kingdom scott_euser

Not sure a good status for this. I guess needs review on the text -> RTBC, then one of the maintainers who does have access can update module homepage.

🇬🇧United Kingdom scott_euser

Thanks for getting all these in and for the handy module :)

🇬🇧United Kingdom scott_euser

Here's an example in case someone ever wants to take this on https://www.drupal.org/project/geofield/issues/2969120

🇬🇧United Kingdom scott_euser

Icons are at the top level, e.g. via /admin/structure/types/manage/page/fields/add-field whereas 'Heading' is nested within 'Plain text' and doesn't need an icon.

(side note, I think he just meant Geofield as an example of how to add an icon)

🇬🇧United Kingdom scott_euser

This is ready to go (though needs 🐛 Fix deprecations, Drupal 12 compatibility Active for the pipeline to be green)

🇬🇧United Kingdom scott_euser

'Oversight' is not entirely fair :)

In Continuation Add Views EntityReference filter to be available for all entity reference fields Active :

  • #110 - This change was suggested by @catch
  • #206/#207 - Breaking change flagged/explained a bit more because numeric allows more than entity reference, and valid things potentially

While those messages are just summaries of slack, we had a lot of discussion with @catch about it it and getting Entity Reference filter in without being used was agred to be the only realistic way to not have a breaking change for existing sites, so we had agreed to deal with the opt-in separately in follow-ups. The main candidate for follow-up was suggested to use 🐛 Views handler loading should respect configuration Active so that existing sites using numeric would respect the configuration and not automatically get swapped to entity_reference, but that turns out to be a big can of worms and ended up getting reverted.

For others stumbling across this issue, for now more sites are using https://www.drupal.org/project/views_core_entity_reference to opt-in to it, or you can opt-in easily yourself as described on that module homepage.

🇬🇧United Kingdom scott_euser

Happy to help you if you need a hand with any of this, can also find me on Drupal Slack (scott_euser)

Production build 0.71.5 2024