🇺🇸United States @mark_fullmer

Awesome. This makes a lot of sense. I was unaware of the `filter` attribute for CKEditor configuration.

However, this change appears to cause tests to fail, where they are not on 7.x (see https://git.drupalcode.org/project/linkit/-/pipelines/517840). Therefore, I'm setting this to "Needs work" to identify whether this filter is in fact interfering with the necessary elements that are allowed on the CKEditor-enabled text format, as indicated by the test output:

The current CKEditor 5 build requires the following elements and attributes: <br><code>&lt;br&gt; &lt;p&gt; &lt;* dir=&quot;ltr rtl&quot; lang&gt; &lt;a href&gt;The following elements are not supported: &lt;a data-entity-type data-entity-uuid data-entity-substitution&gt;',

This issue is certainly worth revisiting, given the development that has happened since this issue was first created. The next step would be to draft a set of steps to reproduce the problem so that a test scenario could be written for evaluating not overwriting the title if it is present.

Add headings

• Clarify language around Composer downloading recipes, by default, to the vendor directory
• Add a tip about how to configure composer to download to the recipes directory
• Provide an example of what applying a Drupal recipe actually does to the site.

I think we should look at RevertOverridesForm too, that might be even more confusing than this one

Agreed, although I'm not sure it's feasible to create a generic statement that accurately reflects what that action will do, given its variability based on site configuration. On one site, this might mean that the entity's Layout Builder sections will be reverted to a single, emtpy, one-column section. On others, "Revert layout" could restore the default sidebar content and order of the main content.

Given this, I think that issue should be dealt with separately.

In Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed

I've changed the target branch from 11.2.x to 11.x. Thanks!

5.x branch and release will be created for this.

Thanks, geoanders, for indicating that there will be Drupal 11 compatibility for Content Export CSV. I would suggest, however, not creating a new major version release for this, given that there are no identified backwards compatibility changes required for Drupal 11 compatibility.

Instead, it would be better for the community if these changes were merged and provided within an 8.x-4.9 release. As has been discussed in detail in 🌱 Guidelines for semantic versioning and Drupal core support Active , creating a new major version simply to allow compatibility with a new version of Drupal core (such as updating only the info.yml core_version_requirement) creates a requirement for developers on any site using this module to do work to change the composer.json requirements. In contrast, a patch-level release should not require any work on the part of developers, since a Composer version constraint should allow automatic updating to a new release.

Since there are over 1,500 sites using that module, this choice has a real impact.

It is tempting to think that compatibility with a new major version of Drupal should be provided by a new major version of a contrib module, but that's not what Semver indicates (https://semver.org/):

MAJOR version when you make incompatible API changes

If there are future API changes planned for this module, I suggest separating that work from nominal Drupal 11 compatibility, providing a 8.x-4.9 release as soon as possible, and working on the 5.x branch as time permits.

Thanks for your consideration!

Joining the chorus -- a new release would be wonderful, given that it is 1 year since Drupal 11 was released. Per the current proposed guidelines at 🌱 Guidelines for semantic versioning and Drupal core support Active , it is acceptable to provide new compatibility for Drupal core versions with a patch-level release, so this release could simply go from 2.0.2 to 2.0.3.

Seconding the RTBC above. It would be great to provide a 3.0.2 release that includes D11 compatibility. Per the current proposed guidelines at 🌱 Guidelines for semantic versioning and Drupal core support Active , it is acceptable to provide new compatibility for Drupal core versions with a patch-level release.

Let's release major alpha|beta version of the module, please

Agreed that a release is well justified, though I'll note that it should probably simply reflect a minor version release from 8.x-3.14 to 8.x-3.15 . Per https://www.drupal.org/project/drupal/issues/3357742 🌱 Guidelines for semantic versioning and Drupal core support Active , this can be provided via a minor release since it does not drop support for currently supported versions of Drupal core.

mark_fullmer → created an issue.

The code change in the MR makes perfect sense, and I think it also reflects how the code should have been implemented in the first place, rather than an affordance for an edge case. In that sense, I agree with the comment in #8, arguing for this in favor of #2864302: Add validation to ensure URL is embeddable → . Merging. Thanks, everyone!

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

With the latest changes, this works fine, tests pass, and this change is not going to negatively affect existing sites. Merging. Thanks, everyone!

I am seeing this issue with 3.0.0-alpha3.

The latest release is 3.0.0-beta2 . Can you check whether updating to that release makes this issue go away? Thanks!

Thanks much, once again!

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

I would like to suggest that supporting aliases is important from a content editor experience point of view - we should be seeking to make Drupal as easy to use as possible. I can see there are probably issues around aliases being changed separately to redirect being created/edited, but I'd humbly suggest that providing the best CX should at least drive a reconsideration of your position here.

Here's a scenario that illustrates "there are probably issues around aliases being changed separately to being created/edited":

1. Content editor creates a node (/node/123) with alias (/policies).
2. Content editor (same or different person) decides that the about page should be retired or replaced, and creates a redirect from /policies to https://external-site.com/policies.
3. Content editor wants to do something/anything with the original /policies page. When the content editor goes to /policies to **edit** that page, they are now redirected and probably quite confused. Even if the content editor knows what's going on, they would now have to find the node in the content listing /admin/content and click "Edit" to be able to edit the content.

I no longer think this change would unambiguously improve the content editorial experience. It comes with its own risks for confusion.

That said, the latest change to the MR adds an opt-in setting to allow redirects when the source path is an alias of an existing internal path. Setting to needs review.

Suggested testing

1. Create a node with a URL alias (/one).
2. Create a redirect from the aliast (e.g., /one -> /two)
3. Visit /one and verify that the behavior of the Redirect module remains as is has been -- /one is NOT redirected to /two
4. Go to /admin/config/search/redirect/settings and turn on "Allow redirects from aliases."
5. Visit /one and verify that is now redirects to /two

Just bumping visibility on this issue. It's now RTBC. This would be a great item to merge and included in a new release, given that it has been indicated as a "Major" level bug. Thanks for the consideration, maintainers!

The maintainers discussed this enhancement request today and decided that it is not something we want to pursue merging into the module at this time. Given that presumably a core solution like Experience Builder will replace Layout Builder, we do not feel it is valuable to add further complexity to what may soon become a legacy solution. We will leave this issue open, but mark as "Postponed," so that others desiring this enhancement can add it as a patch.

if you are blocked by this, consider switching to https://www.drupal.org/project/samlauth → [...] We switched, I no longer use this project and will only provide minimal or no maintenance for this project going forward.

Our team also recently switched from using simplesamlphp_auth + SimpleSAMLphp to samlauth + OneLogin. The transition was simple, the dependencies are now much simpler (since OneLogin does not have a dependency on Symfony, which is where much of the headache of the SimpleSAMLphp v. Drupal stack comes from), and the functionality is equivalent or potentially better (simpler UI for configuring attribute-based role assignment.

Thanks for this very significant amount of work to match static analysis checks!

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

Awesome! Thanks for the PHPCS cleanup!

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

I will gladly work with anyone who wants to contribute, but I have no desire to keep throwing away effort on a project that doesn't seem to be wanted any more.

Thank you again, @tr, for this module's contribution to the Drupal community and to your literally thousands of hours dedicated to maintaining it, equivalent to a substantial amount of wages for a senior developer.

Per 💬 Offer to co-maintain mimemail module Needs review , I am volunteering to co-maintain the module. I have outlined my understanding of your priorities for the module there, which I will assiduously pursue with all practicable expediency.

Thank you for your consideration!

mark_fullmer → created an issue.

mark_fullmer → created an issue.

Thanks so much for the release!

Marking as RTBC, based on testing!

Maintainers, would it be possible to include the changes here in a 8.x-1.0-rc5 release? Given the minimal nature of the changes, there is absolutely zero risk to this introducing regressions for sites, and providing a release will allow more than 2,000 Drupal sites using this module to update to the latest supported version of Drupal core. Thanks for your consideration!

Maintainers, would it be possible to include the changes here in a 8.x-1.0-beta7 release? Given the minimal nature of the changes, there is absolutely zero risk to this introducing regressions for sites, and providing a release will allow more than 1,000 Drupal sites using this module to update to the latest supported version of Drupal core. Thanks for your consideration!

Any update on whether a release is forthcoming? Thanks for all the work the maintainers do!

Adding one more voice to the comments above. A Drupal 11-compatible release is very much desired!

Thanks for the commit and the 2.0.1 release! Marking this is as Fixed!

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

Assigning myself to review questions raised in #70 & #72...

Huh! This problem was introduced during work for Drupal 11 compatibility in https://www.drupal.org/project/linkit/issues/3412457 📌 [Drupal 11] user_role_names() deprecated in 10.2 and removed in 11.0 Fixed . This PHP class definitely needs the Role class declared in its use statements. Thanks for reporting, and for the fix!

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

Yes, the supported version of this module does include a CKEditor 5 plugin and Drupal text format editor that effectively adds a button to the rich text editor for insertion of URLs directly into rich text (i.e. "body") fields. See the module documentation at https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →

Reviewing the code change in the merge request, I don't believe this is implemented correctly. It "fixes" the problem by removing code. Per https://www.drupal.org/node/3436275 → , we should use RenderElementBase instead of RenderElement. We should also probably add a backwards compatibility check per https://www.drupal.org/node/3379306 →

Setting to "Needs work"

Agreed with the rationale in #18. MR 20 replicates the patch that multiple people have confirmed works, and tests are passing, so I'm going to merge this. Thanks, everyone!

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

This addition will not negatively affect existing usage, and if it facilitates targeting elements in automated tests, all the better! Merged; marking as "Fixed"!

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

This introduces test failures, per https://git.drupalcode.org/project/linkit/-/jobs/5001403 . Setting back to "Needs work."

Thanks for the fix!

I've added an MR that is based on the patch from #16 that applies to the 7.x branch. Setting back to needs review based on the change between #14 and #16.

Yes, thanks. Closing as a duplicate on 🐛 Linkit Link field: Double-encoded file links Active -- that one is a bit further along with more discussion.

I think it would be good to add test coverage for this so that this use case is accounted for during subsequent code changes. I am happy to work on that when I have a chance, but if anyone else wants to take that on, that'd be appreciated!

Thanks, everyone!

Alternatively, the fields could be inline by default and responsively become two lines if there's not enough space.

I think this would be preferable, to accommodate various screen widths.

Another consideration would be, for multi-item widgets only, to make each link/title pair be a collapsible fieldset. That probably sounds like a lot for just two text fields, but people will often extend link widgets to add other link options, like "open in new tab" or link icons...

Thanks for the fix! Always good to quiet that log noise!

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

Note that commit https://git.drupalcode.org/project/google_cse/-/merge_requests/16/diffs?... also revises the business logic so that existing sites will automatically inherit the accessibility tweaks without having to update the configuration, but can also opt out of this if desired by the UI configuration.

I note that the functional change in #74 has been lost (it wasn't included in #79).

I added the additional check to the MR via https://git.drupalcode.org/project/redirect/-/merge_requests/133/diffs?c... .

This is again ready for review!

Thanks for the review, @mmarler! I've resolved the two things noted in comment #9 with the latest commits in #11. As for the comment in #10:

Also, looks like I can't uncheck the "Add accessibility improvements to search results" checkbox.

The setting is designed only to be available when the previous setting, "Display search results," is set to "On this site (requires JavaScript)". Is that possibly the issue -- the configuration is set to "On Google"?

mark_fullmer → created an issue.

Not really sure what got me in this state, walked away and it was there when I came back.

For us, this occurred thusly:
1. We maintain a feature module that includes configuration that can be installed newly on multiple sites.
2. The configuration in this module included configuration for a contrib module that subsequently removed that part of the configuration.
3. When we tried to install this feature module using the latest version of the contrib module, we got the error above.
4. This was resolved by removing the no-longer-relevant config from our .yml files.

Reference: https://www.drupal.org/project/facets/issues/3514174#comment-16049977 🐛 Facets throws error on install Active

I agree that the simplest resolution would seem to be to skip the processing if the the attempt to load results in a null object, e.g.

    foreach (['config', 'content'] as $dependency_type) {
      $affected_dependencies[$dependency_type] = array_combine(
        array_map(function ($entity) {
+          if (is_null($entity) || !method_exists($entity, 'getConfigDependencyName')) {
+            return '';
+          }
          return $entity->getConfigDependencyName();
        }, $affected_dependencies[$dependency_type]),
        $affected_dependencies[$dependency_type]
      );
    }

mark_fullmer → created an issue.

Re-opening...

I haven't read the whole issue so please forgive me if I'm restating arguments that have already been made. I did search for phrases related to my post but it would take too long to read the whole thing.

I believe the main resistance to supporting aliases is stated in comment #181 -- the possibility for existing redirects in existing sites to behave differently after this change.

Trying to look for a win-win solution here, what about the support of aliases being an opt-in configuration option? That would avoid the concern stated in #181, right?

mark_fullmer → changed the visibility of the branch project-update-bot-only to hidden.

Basically, assert($form_builder instanceof FormBuilder); doesn't exist in either (beta6 or the module's latest 8.x-1.x), so the patch fails to find the line to remove.

It exists here: https://git.drupalcode.org/project/workflow_buttons/-/blob/8.x-1.x/workf...

It is not in the 1.0-beta6 release, since that tag occurs prior to the two commits that introduced, and then modified that code:

Introduced: https://git.drupalcode.org/project/workflow_buttons/-/commit/a3badee0e30...
Modified: https://git.drupalcode.org/project/workflow_buttons/-/commit/f4d5631ddc6...

So we are not going to have a single patch that applies both to the 8.x-1.x branch and 8.x-1.0beta6.

I've created a new branch / merge request, confirmed it has all the commits present in the 8.x-1.x branch, and replayed the relevant D11-compatibility changes, omitting code syntax cleanup since that should be considered out of scope for D11 compatibility solely.

People coming here with problems with spider traps on the facets should look at the newly released https://www.drupal.org/project/facet_bot_blocker → . I haven't tested it yet, but its methodology allows still using checkbox-style facets (instead of dropdowns -- #99) by rate-limiting repeated queries for facets.

In the case of our site, this was resolved by removing the following configuration for Facets Summary that is no longer part of the 3.0.x version:

-processor_configs:
-  hide_when_not_rendered:
-    processor_id: hide_when_not_rendered
-    weights:
-      build: '45'
-    settings: {  }

This appears to be able to happen if a site has configuration related to Facets 2.x and the site is attempting to install from scratch. This is apparently an assumption made in Drupal core. See related issue in 🐛 Error: Call to a member function getConfigDependencyName() on null Active .

I've updated the MR to be mergeable into the latest changes in 8.x-1.x. Setting back to 'Needs review' -- based on the previous comment, this should be able to be set to RTBC forthwith.

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

But I would really rather that people fix their sites and post known patches here

Yes, I can do that! I can verify the following:

- Using Samlauth 3.11 on its own does not trigger this warning for multiple sites I tested
- Using the Metatag module (see #6 and #8 above) does not trigger this warning, at least not with the configuration we have.
- What *is* triggering this warning is a custom implementation of hook_user_login() that modifies the destination parameter, passing a Drupal Url object as the parameter WITHOUT SPECIFYING that the metadata should bubble:

From Core's API, the Url::fromRoute->toString() method takes an optional parameter that defaults to FALSE:

public function toString($collect_bubbleable_metadata = FALSE) {

In the case of my custom code, this change suppressed the warning:

/**
 * Implements hook_user_login().
 */
function mymodule_user_login($account) {
  $param = \Drupal::request()->query->all();
  if (!$param || !isset($param['destination'])) {
    // For every user but "user 1", redirect to /dashboard upon login.
    if ($account->id() != 1) {
-      \Drupal::service('request_stack')->getCurrentRequest()->query->set('destination', Url::fromRoute('MYMODULE.ROUTE')->toString());
+      \Drupal::service('request_stack')->getCurrentRequest()->query->set('destination', Url::fromRoute('MYMODULE.ROUTE')->toString(TRUE)->getGeneratedUrl());
    }
  }
}

My conclusion is that for the majority of folks coming to this issue, the problem is probably in custom code that implements hook_user_login()

Confirming everything that's stated above: the current base URL does not work with the Android Google Calendar app, and updated to the proposed base URL does work, and the new URL also works in browsers.