#Needs tests

I rebased the MR on 11.x, I had to drop some commits and reverts that were causing conflicts so it'll be easier to rebase later.
We were previously using 🐛 Access denied to published private file if original translation is unpublished Needs review on our project but I can confirm this solution also covers it.
Also attaching static patch for composer.

🐛 Translation is not supported in file module Needs work does seem to cover this, thanks for pointing it @mohit_aghera.

    $this->cssCollectionOptimizer->deleteAll();

This is still in the theme installer.

Needs the patch converted to an MR, or just a fresh MR created removing that line. Not sure we need tests for this, we'd just be testing that we removed the code really - e.g. we'd need to get some CSS aggregates created on disk, install a theme, make sure they're still there. But also with 📌 Bring back the asset stale file threshold Active new aggregates wouldn't get deleted anyway, making this impossible to test. Given that, removing the needs tests tag.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Since steps to reproduce haven't been provided or summary updated going to close out. If still a bug in D11 please re-open

smustgrave → closed merge request !8311

Thanks @grevil! Great (and really hard) work. Current status attached as static patch.

@berdir, thanks for the feedback!

Doesn't seem to work at all on cardinality 1 fields, I'd want to use this for the paragraphs_library paragraph type, which is one of those. I get a button but no autocomplete field

This sounds weird! I have only tested it with target entity type being "node". Could you test the latest MR, if that is still the case now?

should use smaller button classes on Claro/gin, they seem huge.

Definitely. A Colleague of mine could possibly fix up the styling.

add another item and select button to open entity browser are IMHO pretty confusing and not a great user experience. maybe the entity browser button could be on top, maybe both small next to each other with some guidance on which does what?

Ok, I'll take a look on Monday.

edit doesn't seem to use the same dialog system as the regular entity browser widget

Hm, could not reproduce this behavior, but maybe I misunderstand. Could you elaborate

Generally feel free to do some final touches here! This was quite the pain to finalize.

I think the main problem is, that this Widget extends "EntityReferenceAutocompleteBrowserWidget" instead of "EntityReferenceBrowserWidget", and therefore the implementation differs enormously from the original widget.

But this might have a reason, and I am really thankful that this MR was already this far in development. Thanks all! 🎉

Anyway, I think this can get reviewed now. Final touches should still be added. Unsure if we can provide test coverage....

I updated the tests and those are green again.

@rp7

Patch in #34 gave me JavaScript issues on Drupal 10.4.6. event.target.preventDefault(); was being called where event.target was undefined.

That is, because there is no event.target.preventDefault(). There is only event.preventDefault(), see https://developer.mozilla.org/en-US/docs/Web/API/Event/preventDefault.

Manually adjusting the weight in the library is not recommended and messes with code execution timings, which leads to frustrating debugging sessions. Please avoid it.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Patch for 11.2.x.

Since there's been no follow up going to close this one out.

I tested the last patch, but it didn't work for Drupal 10.5.x and Linkit 7.0.8. If anyone has any idea how to solve this, I would greatly appreciate it.

I was able to reproduce the bug in core in a FunctionalJavascript test using NestedEntityTestForm from the field_test module. This test has been committed to the MR of 🐛 DateTimeComputed: Fatal error when Ajax call in form. (strlen() on array) Active . Also see comment #21 on that issue.

I have also updated that issue with an alternative solution that removes the need to modify DateTimeComputed::getValue.

This issue could possibly be closed as a duplicate of 🐛 DateTimeComputed: Fatal error when Ajax call in form. (strlen() on array) Active . But I guess it begs the question, is it right for DateTimeComputed::getValue to expect the value to either be a string or NULL? I propose it is and stricter typing should be applied by throwing a helpful exception. If that is the case, then this issue, if not closed as a duplicate of 🐛 DateTimeComputed: Fatal error when Ajax call in form. (strlen() on array) Active , should be closed as a duplicate of 📌 Remove the try/catch block from DateTimeComputed::getValue() Needs work .

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Attached a patch version of the current state of MR !12445.
This provides a stable snapshot that can be used while the MR is still in progress, especially for projects that need to apply the fix immediately. No code changes were made — this patch reflects the MR as-is at the time of upload. This may be helpful for anyone wanting to apply the fix without tracking MR changes.

Setting strict: true still causes the recipe to fail (but now on a different piece of config). Quoting #3542282-3: Try making XB's `test_site` Check if #3532271 is still blocking → :

Error: Command failed: sudo -u www-data DRUPAL_DEV_SITE_PATH=sites/simpletest/42352967 php core/scripts/drupal recipe /builds/project/experience_builder/_drupal/web/../web/modules/contrib/experience_builder/tests/fixtures/recipes/test_site
In ConfigConfigurator.php line 68:
                                                                               
  The configuration 'experience_builder.xb_asset_library.global' exists alrea  
  dy and does not match the recipe's configuration                             
                                                                               
recipe [-i|--input INPUT] [--] <path>
    at ChildProcess.exithandler (node:child_process:422:12)
    at ChildProcess.emit (node:events:517:28)
    at maybeClose (node:internal/child_process:1098:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:303:5)
    at Process.callbackTrampoline (node:internal/async_hooks:128:17) {
  code: 1,
  killed: false,
  signal: null,
  cmd: 'sudo -u www-data DRUPAL_DEV_SITE_PATH=sites/simpletest/42352967 php core/scripts/drupal recipe /builds/project/experience_builder/_drupal/web/../web/modules/contrib/experience_builder/tests/fixtures/recipes/test_site',
  stdout: '',
  stderr: '\n' +
    'In ConfigConfigurator.php line 68:\n' +
    '                                                                               \n' +
    "  The configuration 'experience_builder.xb_asset_library.global' exists alrea  \n" +
    "  dy and does not match the recipe's configuration                             \n" +
    '                                                                               \n' +
    '\n' +
    'recipe [-i|--input INPUT] [--] <path>\n' +
    '\n'
}

— https://git.drupalcode.org/project/experience_builder/-/jobs/6267192#L1235

(That's the test results for https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...)

Based on a very quick test, some first feedback:

* Doesn't seem to work at all on cardinality 1 fields, I'd want to use this for the paragraphs_library paragraph type, which is one of those. I get a button
* should use smaller button classes on Claro/gin, they seem huge.
* add another item and select button to open entity browser are IMHO pretty confusing and not a great user experience. maybe the the entity browser button could be on top, maybe both small next to each other with some guidance on which does what?
* edit doesn't seem to use the same dialog system as the regular entity browser widget

(we have the widget pretty heavily customized in our project, with custom edit/delete icons on top of the previews instead of buttons, so I'm not 100% sure if anything affects that, didn't test on plain claro yet)

I raised 💬 Provide more help understanding what triggers "Non-translatable fields can only be changed when updating the current revision" Active in core (needs consideration on best approach) to try to find a way to provide more information for developers to be able to understand the cause of the problem in their specific case, please do consider providing suggestions there on how to make this particular symptom more friendly to debug. Thanks!

A few things are still not perfect.

I think the problem here is there are many ways to get to the same symptom so many people get to this issue via Google or other means.

Layer builder asymmetric translations for example doesn't have this module as a dependency as far as I can see.

Ultimately we need to try to solve the problem described in the issue summary and if there are other problems ideally those get raised as seperate issues (potentially in other modules or core if unrelated to this) or it will be extremely difficult for this to ever get merged (and even better, ideally having failing tests to reproduce the issue).

@scott_euser I'm sure that no one ignored you :) But for a module of many downloads, it's normal that in some cases we expect two or three people to say that it's working for them in order to then set it as RTBC. For example, @randalv in #60 described a scenario where it's not working. It might be something not-related to this issue specifically but, in any case, the next one coming over should take it into account and decide whether it's ok to set it as RTBC or not - if he has the experience or the guts to do so.

I also did not get a clear answer on my question in #45 🐛 Non-translatable fields can only be changed when updating the current revision. Needs work ; but I think we can live without an answer.

So, let's keep the good news: It's RTBC! :)

Despite ignoring me plea in #55 the comments seem to indicate this is working well and given we have test coverage for it as well, hoping its okay to move to RTBC even though I did some of the work here

Boldly upping priority because these headers are a foundational part of perf improvement on all Drupal sites.

Just repeating what was mentioned above - this patch successfully stops the behavior of "/foo?" redirecting to "/foo", which was resulting in infinite redirects because of an interaction with caching at the CDN level.

#9++

(which is not a best practice, but it's also not forbidden, and sometimes, there may be a good reason for it)

What I meant is that config exported as default config into a module is (AFAIK) supposed to NOT have a UUID — default config in modules is supposed to get random UUIDs generated — because all that should matter is the config entity ID (a machine name), not the UUID.

What are the rare cases where you would want/need the UUID in a config?

See XB's \Drupal\experience_builder\Entity\XbAssetLibraryTrait::getJsPath() — where it needs to generate a predictable file name based on the config entity UUID.

Lots has changed/been improved/hardened in XB itself and in Drupal core 11.2 — this issue was created before 🐛 PHPUnit Next Major tests failing Active landed, so first double-checking whether this is truly still needed. Although … #9 will definitely remain true 😇, but then it probably would only be .

Doing that double-checking in 📌 Try making XB's `test_site` Check if #3532271 is still blocking Active …

The comment #52 🐛 Bugs if user has blank email address Needs work work it for me! Thanks!

Since there's been no follow up going to close this one out. Also going to mentioned field_layout is marked to be deprecated and will be moving to contrib in Drupal 12.

What are the rare cases where you would want/need the UUID in a config?

the recipe data contains a UUID (which is not a best practice, but it's also not forbidden, and sometimes, there may be a good reason for it)

Wim, can you elaborate on this bit? It's not forbidden but that might just be an oversight on our part. What are some good reasons for a recipe to include config UUIDs?

Cleaned up non-ASCII characters from the patch. Updated code attached for review.

Is there any reason we shouldn't just unconditionally remove the uuid and _core keys from the incoming recipe data?

The recipe system assumes those keys aren't there, but that's not a safe assumption -- lots of people make that mistake. We definitely do not want to have those keys be considered in the comparison.

On the other hand, we could remove those keys from the recipe data during the comparison, but if the recipe still has those keys, they'll get imported anyway...which could be bad.

I think what needs to happen here is that RecipeConfigStorageWrapper should unconditionally remove those keys from any config it reads in. Either that, or the FileStorage object created by \Drupal\Core\Recipe\ConfigConfigurator::getConfigStorage() needs to do it.

I guess we should focus on getting this into 2.x. Since there are some substantial changes this needs a reroll.

Updated the patch to fix coding standards issues reported by phpcs.
Please review the attached patch.

Updated the patch to fix coding standards issues reported by phpcs. Please review the attached patch..

How did I end up reporting this?

See #3532130-11: `/xb/api/v0/config/component` crashes when `article` node type does not exist due to hardcoded assumption in `GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo()` → for what prompted me to add strict: false to XB's test site recipe and also prompted the creation of this issue.

The 3 commits before that comment show what had to change.

The problem IIRC is that the recipe system when strict: true (the default), the comparison of the UUID stored in tests/fixtures/recipes/test_site/config/experience_builder.component.js.xb_test_code_components_using_imports.yml fails to match the active one.

FYI this would allow XB's test recipes to become more reliable:

diff --git a/tests/fixtures/recipes/test_site/recipe.yml b/tests/fixtures/recipes/test_site/recipe.yml
index c776203de..d476bf6a1 100644
--- a/tests/fixtures/recipes/test_site/recipe.yml
+++ b/tests/fixtures/recipes/test_site/recipe.yml
@@ -8,10 +8,6 @@ install:
   - xb_test_sdc
   - xb_dev_standard
 config:
-  # Loose config imports because the code components in `xb_test_code_components` contain UUIDs, which are used to
-  # generate corresponding assets on disk with predictable names.
-  # @todo Change to `strict: true` after core bug https://www.drupal.org/project/drupal/issues/3532271 is fixed.
-  strict: false
   import:
     experience_builder: '*'
     system:
… which should also make things for Drupal CMS a whole lot more predictable :innocent: 

And I can't imagine us being the only ones affected, so bumping priority 😇

This might be a nice novice task to learn JS refactoring to core/once and implementing tests.

I need some time to review this MR.

Who can test it manually?

MR is now properly rebased on top of 5.x.
@ivnish or other, may we set it as RTBC? From me it's a yes of course.

I've rolled #22 into an MR as that's the patch we've been using in prod for years. I like the idea of #60 but that'll have much wider reaching BC issues for sites using getParentEntity so I'm not sure how viable that'll be. Still NW for tests.

one test that might be considered adding is simply checking the skipto.js file for all the key values that are currently added to the skipto module and also those that will be added soon. that way it would be a fail safe that not one default value or default key changes at one point. that way it would be safer to update to a new version of skipto.js

@vansebroeck looks like this should always return an array (plural!). If still relevant, please create a MR.

Still makes sense, so maybe proceed in a MR?

@mbovan I still like the idea. Maybe turn this into a MR to proceed?

wanted to bump 1 more time.

@oknate thanks - should we get this fixed as MR?

Added test coverage for mailto scheme. Moving to NR.

@riyas_nr I haven't done an in-depth review of the MR yet, but I notice we're adding a condition for mailto: URLs. There's no test coverage for it though. Is it possible to add a couple of test cases to LinkItemUrlValidationTest::getTestLinks() for them?

I wonder if we should move this to the Token module?

In the previous patch the URL constraints failed because the value was passed directly to the parent validator.

Updates in this patch:

1. Created a Symfony Url constraint object and passed it to the parent validate().
2. Added allowed protocols and used the child constraint message.
3. Applied the constraint only for external URLs.
4. Added test coverage for invalid characters, multiple comma-separated URLs, and other edge cases.

This ensures external links are properly validated and the behavior is covered by tests.

I made a new MR based on MR 159 that adds support for relative URLs alongside absolute URL functionality.

So now you should be able to use the following tokens:

• [node:translation:en] -> Defaults to absolute
• [node:translation:en:absolute]
• [node:translation:en:relative]

Note that the options are expandable in the token tree so you don't need to guess it for yourself.

Making the patch into a merge request (MR) increases. As the issue is tagged with needs tests and patch(es) contains none do I push this back to NW.

Back to NW as of #32 - maybe someone could start a fresh MR based on that?

Automatically closed because Drupal 7 security and bugfix support has ended → as of 5 January 2025. If the issue verifiably applies → to later versions, please reopen with details and update the version.

Since there's been no follow up and was tagged as a minor going to close this one out. If still a valid task please re-open.

Thanks all!

I keep running into this error as I apply my new theme to more sites. But now I've had a WSoD crash because of it. I'm altering Views exposed forms to format them as USWDS Search components. It involves applying a few classes, changing the text input type to search, and setting role="search" on the form. For some reason, that last one, setting the role attribute, really caused a problem when the exposed form is in a block and that block has its title display on. template_preprocess_block() (this site is still in D10) tries to set a unique ID for the block with Html::getUniqueId(). When the role attribute is bubbled up to the block due to this issue, suddenly the preprocess function starts passing the title render array to getUniqueId(). Then the error is thrown because the expected input is a string, not an array.

I didn't bother trying to figure out what weird code path causes that to happen. I'm fortunate that I can simply turn the title display off in this one instance. I just want to point out that there are scenarios where this isn't harmless. I'm lucky that I knew about this issue so I could work out what was going on.

Since there's been no follow up, going to close this one out. If separate from #11 please re-open updating the summary to use the issue template.

Thanks!

Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.

Since there has been no activity here for over 8 years we are asking if this problem persists on a currently supported version of Drupal. To help, add a comment explaining if the problem still occurs or not. Any extra detail you can provide can help others who experienced this.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!

Verified the changes :

Tests fail because of 🐛 Fix arguments for tests Active .

There's another duplicate that was filed as a bug report due to protocol-relative URLs being accepted input that were validated as internal URLs, but resulting in malformed links when rendered: #2889658: Handle protocol-relative URLs in LinkWidget → .

I rerolled the patch from #155 for Drupal 10.5.2

er.garg.karan → made their first commit to this issue’s fork.

Uploaded a new patch that works with 3.1.0 version

This should be it! Please review!

Modified patch for D11.2

Also I added the trait method to the .phpstan-baseline.php like has been done for other tests using the trait. Maybe it would be better if we added a void return type to the ContentModerationTestTrait and generated a new baseline since it's test code rather than runtime code?

I added the related issue where we are doing this everywhere. There is a bunch of chatter in that issue about whether it's safe to add the return type to traits if there might be subclasses overriding the trait methods. Since this trait is used in ResourceTestBase there is a chance that this becomes problematic. Let's not hold up this issue to discuss that, and stick with what you've done instead.

Since the rebase in #86 there were some upstream changes to PHPStan. Notably $height and $width were removed from the baseline and return types were enforced for all new methods which caused an error with the trait we were using:

https://www.drupal.org/project/drupal/issues/3461318 ✨ Enforce return types in all new methods and functions Active
https://www.drupal.org/project/drupal/issues/3347502 📌 Fix PHPStan L1 errors "Access to an undefined property Foo\Bar::$baz" Active

I removed references to $this->height and $this->width and replaced them with $this->get('width')->getValue() and $this->get('height')->getValue() like they did in the related core issue.

Also I added the trait method to the .phpstan-baseline.php like has been done for other tests using the trait. Maybe it would be better if we added a void return type to the ContentModerationTestTrait and generated a new baseline since it's test code rather than runtime code?

Also, the IS is out of date since a new ImageFieldItemList was removed in #74 so I removed it from the issue summary too.

This is a must have.

We are using Url::fromUserInput in the DomainSourcePathProcessor of the Domain module and it causes a significant performance issue (besides some annoying loops).

Automatically closed - issue fixed for 2 weeks with no activity.

No worries, thanks for reporting back. And good to hear it works for you, too.
Merged.
Thanks again, everyone!

simohell → changed the visibility of the branch 3301173-allow-starterkit-theme to active.

Could we have a MR on 2.0.x please ?