🇧🇪Belgium @svendecabooter

Taking the liberty to increase priority of this issue. Feel free to revert if you disagree.

I have also tested the "Show open by default" and "Auto-collapse" functionality.
This seems to work as designed.

However, it seems the auto-collapse setting can still be manipulated, even if "Show open by default" is checked.
In the form display settings, if "Show open by default" is checked, then the "Auto-collapse" setting disappears.
However, you can uncheck "Show open by default", then set "Auto-collapse" to on or off, and check "Show open by default" again.
The settings checkbox disappears, but the setting remains saved and does seem to impact the form.

So either both checkboxes should remain visible and work independently of each other, or the "Auto-collapse" should be automatically set to "Off" if "Show open by default?" is checked...

Now the behaviour is a bit confusing.

svendecabooter → created an issue.

@jurgenhaas:

I tested this patch on webform 6.3.0-beta5, and Drupal 11.2.5.
This does not produce this or a similar error.

That would make sense I think, since the Webform logic declares a more specific function return type, overriding the less specific core method, not providing a return type, which is valid.
In Drupal 11.2.6 however, core does provide a specific return type, where Webform makes it less specific (without this patch).

Created a follow up issue for the last comment: https://www.drupal.org/project/custom_field/issues/3556431 →

svendecabooter → created an issue.

There is also a bunch of logic regarding relationship status:
- Filter on status in RelationshipListBuilder
- \Drupal\crm\Controller\RelationshipController::getActiveRelationships() vs \Drupal\crm\Controller\RelationshipController::getInactiveRelationships()

This should all be removed then?

Would this also imply removing the "status" base field from these entities?
I can see a use case where we want to mark a specific contact as active or not (i.e. archived).
But that field should probably not be linked to the publication status of the entity, as suggested.
Maybe it should also rather be a list field, so more than 2 entity states can be defined.

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

Updated the contrib record with other people present at the table working on la_eu stuff.
Marking as fixed.

When the navigation module is not installed on D11.2+, I do not see the link show up anywhere else.
Tested on an Drupal 10 site as well, and the link doesn't show up either.

So from what I can tell, the addition doesn't break anything in case the link is not relevant. Unless I'm missing something.

This basically provides a second route that is identical to the masquerade.unmasquerade route, but without the CSRF check.
This seems like useless duplication to me.

Either we decide the CSRF token is a necessity for security purposes, and do not add another link that circumvents those security concerns, rendering the added security to the masquerade.unmasquerade route useless.
Or we decide the CSRF token is not really needed, and removing it does not introduce a security risk. In that case the original masquerade.unmasquerade route can be changed to remove the CSRF token requirement.

Tested the MR, and this adds the "Unmasquerade" link to the user menu in the Navigation bar:

This seems like a logical place, and helps with easily unmasquerading.

I have tested this MR.

One issue I noticed:

The "Collapse all" button is shown for all variations of the "Wrapper" setting, where it only actually does something when the "Wrapper" setting has been set to "Details". Probably the button needs to be hidden in the other scenario's, unless collapsing those is also possible?

Furthermore it would be nice if we could optionally select a (text) property of the custom_field field that functions as the details label, instead of
Item (1), Item (2), etc...
But I guess that should be a followup issue?

I fixed the TestWith attribute.
Failing tests are functional javascript tests now, related to open telemetry. Doesn't seem related to this MR.

Tests seem to be acting up...
They are passing locally though, so setting to Needs review until further notice.

Note also that the numeric block_id and block_revision_id get exported, which is not really interesting for use in recipes, since that ID could be in use by another block already when importing the exported content via a recipe.
I'm not sure if the import system will strip that out or not, but on Drupal 11.2 with contrib default_content I had to jump through a bunch of hoops to avoid that being an issue.

svendecabooter → created an issue.

Thanks for the MR!
I finally got around doing a review, and have added comments on the MR.
Can you have a look at those, and update where relevant?

I helped setting up pipelines with kraut (mostly just pushing 1 button ;)), tried to help Greece and Germany, and tried getting the drupal.be website up to date with upstream.

I manned the booth and handed out candies :)

Improved the multiple invocations test coverage as well.
Tests keep failing, but the failures seem unrelated to this functionality.

The duplicate array values is still an outstanding issue though...

Added support for non-array values now.

If there are no objections, you can add me as an administrator for Drupal BE:
name: Sven Decabooter
email: info@drupal.be

👋🏻

👋🏻

👋🏻

👋🏻

I noticed another potential issue when testing this: should we account for duplicate array values?

Take for example:

mymodule.settings:
  simpleConfigArray:append:
    property: something
    values:
      - a
      - b

Given that recipes are idempotent, and thus can be run multiple times, this then results into config like:

mymodule.settings:
  - a
  - b
  - a
  - b
  - a
  - b

(if the recipe / config action gets applied 3 times)

As discussed on Drupal Slack with phenaproxima, I also think support for config entities is out of scope here.
Adding quote by phenaproxima here for future reference:

The presumptive reason being that doing this for config entities would be a more semantically meaningful action that might call for a completely dedicated, specialized action per use case, rather than generic array manipulation.

• Invoking the config action with a non-array value does not work currently. This needs to be added, with test coverage.
• Multiple invocations do work, and there is the testSimpleConfigArrayMultipleInvocations test. It only tests multiple invocations to the same property though, not to separate properties.

svendecabooter → created an issue.

Linking the issue in the custom_field issue queue, where this functionality could be added

This seems to fix D11 compatibility.
Can this be merged and a new release tagged?

+1 - this module is blocking us from upgrading a site to D11.

Can this be merged, and a new release tagged?

The dependency change is probably not needed.

I thought it would remove the ai_prompt entity upon uninstalling the ai_translate module, but after some extra testing, it seems it does not.
It just lets the entity exist. But the prompt type does get removed, so a stale ai_prompt entity remains, without a valid prompt type.
Not sure if there is a way to get that entity removed as well (if that is desired).
Maybe general logic for deleting ai_prompt entities when their dependencies are removed, is something for a separate issue?

Created a new branch with fixes I noticed were missing in the merged functionality.
Could you review and add as well?

Test seems to fail - still needs to be addressed.

svendecabooter → changed the visibility of the branch 3.x to hidden.

svendecabooter → changed the visibility of the branch 3010914-product-variation-access to hidden.

svendecabooter → created an issue.

@valthebald

This logic was taken from the documentation in the AI module.
Seemed logical to me, but indeed the update hook would just need to focus on the prompt that is currently active on that particular website, not what the module ships with by default...

svendecabooter → created an issue.

svendecabooter → created an issue.

Hi richgerdes,

Thanks for the feedback. Moving this back to the original queue.

I haven't been active in the queue indeed. We're using the module on a few sites, and it mainly works as intended, so no direct need.
The most urgent need is a D11 release indeed, so I'll check if I can get that out of the door.

I'll then look at the roadmap to see if I can assist with moving some of these things along.

If there is no response from the project maintainers, I'd be willing to volunteer as co-maintainer as well, to move this module forward.
I can opt projects into security advisory coverage.
I could then look into working together with sokru where relevant or appropriate.
Let's see if there is any response here.

Moving to the Drupal.org project ownership queue, as per instructions in https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... → , since there has been no reply for 14 days.

Yes, since 1.2.x requires ^10.4 || ^11, I figured it was safe to use those.

See draft MR for a proof of concept of how this could work.

This would validate different prompt types differently.
See screenshots from output of the Config Inspector → module:

ai.ai_prompt.suggest_tags__suggest_tags_default
Uses general ai.ai_prompt_text.* schema

ai.ai_prompt.ai_translate__blaat
Uses specific ai.ai_prompt_text.ai_translate schema

svendecabooter → created an issue.

svendecabooter → created an issue.

svendecabooter → created an issue.

The reason why the '#parents' property might need to be added, might need to be clarified, since I don't know exactly.
Is it because the form element is rendered within a tree structure, that it might be needed?
Or because there might be multiple ai_prompt form elements on the same form page?

I had a scenario where both of those were true, and adding it made everything work for me, but I don't know which of the scenario's it fixed exactly :)

svendecabooter → created an issue.

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

Wouldn't it be a better DX if there were a dedicated config action in the Key module?

Then developers would more easily find out how to do so.

Updated the user stories in the original description, for extra clarification.

All dependencies of this module now have a release, so I tagged an alpha2 version.
This should allow the module to work without patches.

Thanks for considering linking to it from the project page.

Thanks for the heads up.
It seems to work fine on my clean install, but perhaps I have another library activated that uses the once library on the page.
Closing this issue again, in favor of the followup issue 🐛 Add drupal/once library dependency Fixed

svendecabooter → created an issue.

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

Thanks for the MR! Merged now.

svendecabooter → created an issue.

svendecabooter → created an issue.

svendecabooter → created an issue.

Thanks, fix merged!

Thanks, fixed now.