🇺🇸United States @dcam

The problem here is we're dealing with with a formatter that does two very different things.

Honestly, as I've been working on the Link module I've been thinking that I wish the plain text URL settings had been implemented as a completely separate formatter. I don't know that there's a way out of it now.

In my opinion this isn't a bug. Wanting to link to a page that can give a 403 response, for instance prompting users to authenticate to view content behind a pay wall, is an equally valid use case. That's borne out by the fact that this is made optional with the formatter setting. It isn't only for the sake of backward compatibility. It's because this is a thing people would actually want to do.

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

This is a duplicate of ✨ Link field > Link display for internal links when no link text is given should not just display node/{node ID} Needs work . Although this issue is older, the other one has had work performed to create a patch.

Removing the Needs Tests tag because I think that's probably covered now.

I have a couple of problems with the new constraint and validator. So I went back through the history of this issue and discovered that they stem from this code being imported from a custom project.

First, the constraint class name is LinkUriValidConstraint, but its message says "The path %value contains invalid characters." Checking for invalid characters is certainly part of validation, but is that all of it? Or does the Symfony constraint also check URL format? The Symfony docs say "Validates that a value is a valid URL string." It seems like the constraint's label and message need to be updated.

Second, the validator class is doing two separate things:

• Using Symfony's UrlValidator
• Checking mailto URLs

Yet both cases end up with the same constraint message about invalid characters, even though the mailto validation may fail for other reasons. We could add a second message to the constraint, but I think I'd prefer to move this code to separate constraint and validator classes.

Yes, I'm still working on the review.

I left feedback on the MR.

I verified that this is the correct CR URL for that issue. The diff is clean. A grep -r "3519185" ./ of the entire code returned zero results. I don't think there's anything to do but RTBC this.

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

This was assigned the wrong subsystem.

If this is about the \Drupal\Core\Url class, then it seems like this belongs in the base system.

I converted #22 to an MR.

In the #4 patch $element['attributes'] got confused with $elements['attributes']['#attributes']. This issue is about removing the latter.

But then the schema for $element['attributes'] got removed in #4. That caused D6 migration errors, which makes sense.

The MR should only be about removing the unused array keys from the value element.

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

I addressed the feedback.

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

WorkspaceEntityOperationsTest is failing.

I'm checking all RTBC issues to ensure their new tests use attributes. Because these changes are minor and the tests are passing I'm going to leave the issue at RTBC.

I'm checking all RTBC issues to ensure their new tests use attributes. Because these changes are minor and the tests are passing I'm going to leave the issue at RTBC.

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

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

I'm checking all RTBC issues to ensure their new tests use attributes. Because these changes are minor and the tests are passing I'm going to leave the issue at RTBC.

I'm checking all RTBC issues to ensure their new tests use attributes. Because these changes are minor and the tests are passing I'm going to leave the issue at RTBC.

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

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

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

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

I converted the tests to use attributes. Since this is a minor change to correct test metadata and the discovery test is now passing I'm changing the status back to RTBC.

> GaborHojtsy: it might well be dead code

This isn't far off. LinkWidget essentially throws away any value that's stored in it. See 🐛 Link options attributes removed on save Needs work . My proposal is to update LinkWidget::massageFormValues() to fix this. I'm not sure this will add all necessary tests for the attributes, so I'm leaving this issue open.

@prudloff thank you for working on this. I left a comment on the MR due to some code duplication.

The other feature request was closed as a duplicate.

I'm closing this in favor of #2889658: Handle protocol-relative URLs in LinkWidget → since this issue evolved into being a bug. The changes in that issue are nearly identical in function. Credit has been granted for this issue.

dcam → changed the visibility of the branch 9.2.x to hidden.

This is a duplicate of 🐛 Use rawurldecode() in LinkFormatter to correctly display encoded URLs Needs work . The other issue is classified as a bug and some work on a fix has been performed. So I'm closing this one.

This error came up for me at work a couple of weeks ago. So I'm familiar with it. I told the person running the security scanner to mark it as a false positive. The OWASP fact sheet that is linked in the issue summary was updated in 2023 after this issue was created.

Update 2023 - this is fixed in modern, evergreen, browsers
Links that use target="_blank" now have implicit rel="noopener" in modern browsers, so this vulnerability isn’t as widespread and critical as before. This implicit rule is also a part of the HTML standard. According to Caniuse.com evergreen browsers support implicit rel="noopener" from about 2018, but there are still some browsers out there that doesn’t support it, so please consider your userbase when/if deciding to drop rel="noopener".

So this hasn't been an issue in browsers that Drupal supports since 2018. Furthermore:

Using rel="noreferrer" implies also rel="noopener", so if you have chosen to use rel="noreferrer", the use of rel="noopener" isn’t required.

I'm tagging this issue for needing security review. I'd like for someone on the security team to weigh in on whether this is even necessary anymore. Currently, I'm inclined to close it as being outdated.

I've decided to postpone this issue on the resolution of 🐛 Display meaningful error messages according to the link type Needs work . The issues are modifying one of the same error messages. But the other issue is classified as a bug because the error message is misleading and confusing. I thought about trying to merge the issues, but I think I'd rather just wait on this one.

Also, I realized that there's an easy workaround for this issue in the Inline Form Errors module. Though I admit that has problems, see 🐛 Invalid path error message is displayed three times in link widget for external link Closed: cannot reproduce .

I looked over the changes a few times. I don't have any feedback. They look good to me. Tests are passing. I wondered if it would need deprecation tests, but another recent issue → that changed a constructor's parameters didn't require them. So I assume we're good there. Finally, there are no other instances of CommentLinkBuilder::__construct() being invoked. I'm going to RTBC it.

I hope you don't mind that I filled out the change record description with before and after examples. They shouldn't be necessary because people should be depending on the service, but I guess it's better to be safe than sorry.

I can't speak as to whether this change is appropriate or not, but your code changes look good. Great job adding simple assertions to existing tests for it.

The only thing that I think is missing is a change record. As noted, this could affect existing sites and that's a red flag to me that we need a notice to developers with before and after examples of the HTML.

Tagging for framework manager review due to the issue with contrib compatibility described in #42.

Support for serialized options was deprecated in 8.7.0 and removed in 9.0.0. Here is the change record for this change: https://www.drupal.org/node/2961643 → . The unserialization code was removed by #3097422: Remove all @deprecated code in the link module → .

In short, this issue appears to be outdated.

The basic functionality of the change works.

When I install the Standard profile the block_content Body field storage is still automatically created because the config was copied to the profile.

When I install the Minimal profile the Body field storage is not automatically created. I can install the block_content_storage_body_field module to get the field storage.

I fixed a typo and a copy/paste error in the CR.

Tests are passing. My feedback was already addressed. This one looks good to me.

I updated the CR. I believe that I made it clear that this change only affects SqlContentEntityStorage.

LinkFieldTest failed on the "Text-only links" section of doTestURLValidation(). The code to remove "route:" from the links before displaying them in the widget was removed. This doesn't only apply to routes like views. It also applies to the special routes. Lines 160-164 of LinkFieldTest would need to be updated like this:

      // Text-only links.
      '<nolink>' => 'route:&lt;nolink&gt;',
      'route:<nolink>' => 'route:&lt;nolink&gt;',
      '<none>' => 'route:&lt;none&gt;',
      '<button>' => 'route:&lt;button&gt;',

To be honest, I don't like this very much. I don't think "route:" should appear before these links. It may be confusing to users. In my opinion we should continue to strip "route:" from them.

Restoring status.

Again, no need to credit me for this. All I did was hit the rebase link. I don't know why it thinks the patch doesn't apply, but it rebases cleanly.

I've verified that MR 13299 is compatible with the link_class, link_target, and link_attributes modules. It is NOT compatible with menu_link_attributes.

...I wouldn't have expected contrib modules to be relying on a #type => value that wasn't working in core, so it'd be good to understand why the contrib modules broke due to this.

They aren't relying on the value. They are relying on the $element['options']['attributes'] render array keys. There's no problem while the Core widget sets $element['attributes']. Nothing else uses it. But the contrib modules' widgets all set up their render arrays to match the expected structure of a URL's options array. They make actual form elements at $element['options']['attributes']. When Core suddenly started doing it too everything broke, probably because Core set it as a value element. But then you had a module like link_attributes turning it into a details with child elements for the different attributes. It's a wonder that they weren't more broken than they were. That's why I decided to leave the Core widget's structure as it has been. Instead, I'm merging the attributes into the field value in massageFormValues().

menu_link_attributes is different because it merges its attribute values in at https://git.drupalcode.org/project/menu_link_attributes/-/blob/8.x-1.x/m.... So we end up with the same "additive" problem described in #25. I don't know if something can be done about this. I'm looking for other opinions, so I'm setting this to Needs Review.

I haven't written a change record pending a decision on a final approach to fix this.

Added screenshots

Updated the IS.

@oily that's some nice work on the test. I'm setting the status to Needs Work for all the other things that need to happen in the issue.

That was an update test. But there was functionality added to the ThemeInstaller that also needs to be tested.

What would you like to see tested? If other settings are kept? Not sure what you are asking more coverage for.

Sorry, I was talking specifically about the change to ThemeInstaller. The update test looks good. But #25 caused the addition of new functionality. I think that a Kernel test that verifies third-party settings are removed/retained from a theme's settings when it's installed is necessary. I looked for an existing test for that, but didn't find one. I might have looked in the wrong places.

I also rebased and did it manually to add the use statement.

Ha ha, that's good. I was just working in GitLab and obviously forgot about the use statement in the suggestion.

It all looks good to me now.

I left a suggestion on the MR for attribute conversion.

I checked the change record. It doesn't contain enough information, particularly the title:

Using the 'access callback' key in the definition is deprecated

Unless you read the related issue field and know where HandlerBase is, then you have no idea what this is talking about. The questions "What part of core was changed?" and "What is 'the definition'?" need to be answered in the CR.

edit: I see in the IS that it's still a pending task to update the CR.

Modifying a theme's configuration when it's installed seems like something we should have tests for. Otherwise I think the changes that resulted from the increase in scope from #25 look OK to me, with one minor bit of feedback.

The feedback was addressed.

Added test attributes.

This was marked as needing tests in #18. None have been added, so I'm restoring the Needs Work status.

dcam → changed the visibility of the branch 2871217-avoid-error-when to hidden.

This is unblocked now.

Attributes and a change record have been added.

Rebased following the move of File module template preprocess functions to a Hook class.

I only read over the changes in the MR, but I noticed a couple of things:

• The deprecation of BlockPluginInterface::BLOCK_LABEL_VISIBLE needs to be added to the proposed resolution section of the issue summary.
• I saw some instances where the text case of false was inconsistent in config files, mainly core/profiles/demo_umami/config/install/core.entity_view_display.node.page.full.yml and core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml. I'm uncertain how important it is. My guess is that there's no linter rule for this or it would have been triggered. But someone may want to double-check all of them.

@luismagr starting a new MR was the right call.

Thank you for working on this bug! It's going to need some more work.

To start, we require all bug fixes to have tests so that we don't end up with a regression later.

Because this is a change to configuration, it will need an update path so existing sites will get the changed configuration. That update path will also need to be tested.

I'm fairly certain this issue will need a change record, but I could be wrong.

Finally, the issue summary needs to be updated with all of the headings from the issue summary template → . You may omit text under headings that are irrelevant to the issue or enter "N/A." It also needs to have thorough steps to reproduce the issue starting from a vanilla install of Drupal. Doing this ensures that someone who is reviewing the issue later on can check it quickly without needing to do any guesswork.

dcam → changed the visibility of the branch 3546589-incorrect-format-in to hidden.

Since the change to CategorizingPluginManagerTrait was reverted now CategorizingPluginManagerTraitTest is failing.

I've updated the issue summary and CR with the new name.

Thank you. Sorry I forgot about doing that.

I modified the constraint validator Unit tests to ensure the error path is set to the uri element. I also added an entire test for the LinkTypeConstraintValidator which I then partially added to the pending bug fix in 🐛 Check of $value not covering LinkItem cases in link contraint validators Needs review . So when one of these two issues gets committed the other will need to be rebased and have the changes merged in.

Feedback has been addressed. Setting to Needs Review for a 3rd opinion on the change.

The test fix was committed. This is unblocked now.

The feedback was addressed. Thank you to @smustgrave for prioritizing the review of this issue.

That isn't a random test failure. I didn't look into it, but it appears to have something to do with Olivero's third party settings. So it seems likely that it has something to do with the most recent changes. Because of that I didn't review those changes.

Hiding patches.

I converted the patch in #50 to an MR, undid some of the code style changes, addressed the feedback in #33, and updated the IS.

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

I renamed the new service and Views area plugin. I saw the other open issue about making a dropdown widget and decided that it would be best to not lock the implementation to any individual widget. This way you can add other widgets as options to a single area plugin, even add a widget plugin system if you wanted, and only have to change the class internals.

Apparently these changes (or similar) are included in 📌 PHP 8.4 compatibility Active , but I didn't realize it because the people working on it lumped changes not related to PHP compatibility into the MR. So there's duplication. The maintainers will need to decide what to do about it.

dcam → created an issue.

The test failures are unrelated. Currently HEAD is failing on Drupal 11. I'll open up another issue to fix them.

dcam → created an issue.

I'm not sure if it's relevant, but check out 🐛 Prefix is shown before description Active . The prefix and description are in the wrong order in form-element.html.twig. It might have implications for this issue.

Updated the IS per #68.

@smustgrave I'm experiencing the issue with quicktabs version 4.0.1.

I have a set of tabs. Each tab contains a view. All views have a contextual filter on the content ID from the URL.

If I set the tabs to load all tabs on page view, then there's no problem.

If I set the tabs to only load the first tab on page view, then click on another tab, no content is loaded. I can see the "Loading content..." message pop up briefly, but it's replaced with nothing. The tabpage DIV remains empty.

Supplying the tab with an argument doesn't fix the problem. I wondered if that's because I have path aliases set for all these pages, but it doesn't sound like it fixes the problem for unaliased pages.

I converted the Functional test to a Kernel test and updated the proposed resolution.

@dcam should that one be closed for this or vice versa?

I think I was still trying to figure that out. Finding that other issue led to a bit of a rabbit hole of issues. There's a larger problem with external URL validation in general.

dcam → changed the visibility of the branch 11.x to hidden.

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

I converted the patch in #2 to an MR and added Unit tests.

dcam → changed the visibility of the branch 11.x to hidden.

dcam → changed the visibility of the branch 3387013- to hidden.

Postponing based on #17.

I don't know the internals of Layout Builder well enough to understand what this install function does. So I'm not comfortable OKing the changes. But I did notice that the old code saved the display entity. The new code does not. That seems like an oversight. If the tests pass despite that, then it probably indicates a lack of test coverage.

The changes look OK to me with the exception of one thing. I left a suggestion on the MR.

I don't know why the NR bot thought the patch couldn't be applied. I rebased the MR again to be sure.