🇺🇸United States @mark_fullmer

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.

Thanks for reporting this! I'll just do a couple manual tests and then presumably commit this & publish a new release.

I am not sure what the projects's technical plan is so don't know what the next step would be - keep supporting GeneratedURLs or drop the support?.

Per https://www.drupal.org/project/linkit/releases/7.0.0-alpha1 → , the 7.x version of this module removes support for use of \Drupal\Core\GeneratedUrl .

Thanks for pointing this out. This looks to be a chunk of code that didn't take into account the switch to using GeneratedUrl in plugins -- https://git.drupalcode.org/project/linkit/-/commit/d07b0c38ef6bd18389dbd... .

Agreed with #30 that what to do with deleted nodes should probably be kept out of scope, as this proposed change does not introduce that problem.

If you want a already defined template (in your case, field-node--field-images--output.html.twig) to be located in a Drupal module, you need to use hook_theme() to tell Drupal to scan the module for that template. As with many cases, Drupal core code gives good examples. See core/modules/node/node.module for how it registers field__node__title, with a corresponding core/modules/node/templates/field--node--title.html.twig

In your case, it would be something like the following:

/**
 * Implements hook_theme().
 */
function katalogsuche_theme($existing, $type, $theme, $path) {
  // Register templates defined in /templates.
  return [
    'field_node__field_images__output' => [
      'base hook' => 'field',
      'template' => 'field-node--field-images--output',
    ],
  ];
}

See similar discussions at https://stackoverflow.com/questions/37643897/drupal-8-custom-block-modul...

In contrast, hook_theme_registry_alter() does NOT tell Drupal to scan your module directory for a given template.

Update summary to disambiguate from Functional tests.

Update summary to disambiguate from FunctionalJavascript

Update the title for consistency with other pages

• More textual cleanup
• remove the outdated content about including 3rd party libraries

Clean up language and example code.

Clean up information architecture

Stub out "Troubleshooting" section

Remove blank ul li item

Add documentation for use with PHPUnit v10

Add example for use with PHPUnit 10 + Drupal 11

I don't really feel like that rename is in line with my intentations for this issue. I'm specifically asking for a change on drupal.org, not guidelines within the current behavior.

Okay, understood! I've reverted the title.

Remove table of contents -- already provided by sidebar

- Link to "Changes required for PHPUnit 10 compatibility" under "Where to go from here"

- Add table of contents

Since this issue really seems to be scoped to guidelines about semver for contrib projects when their code changes the relationship to Drupal core, I think it would be more actionable for people if it didn't try to cover all scenarios for major/minor/patch releases. The following narrows the scope here to only prescribe behaviors when code changes its relationship to Drupal core. I've updated the issue description with this proposal, too:

A contributed project should release a new major version when its code changes the relationship to Drupal core by:

• Dropping compatibility for a currently supported version → of Drupal core
• Introducing breaking changes from Symfony or another dependency where backward compatibility (such as adding types to method signatures) cannot be supported.
• Introducing impossible dependency resolution (such as the module requiring Guzzle 7 when a previous Drupal core version required Guzzle 6).

A contributed project should release a new minor version when its code changes the relationship to Drupal core by:

• Dropping compatibility with an unsupported version of Drupal core → .

A contributed project should release a new patch version when its code changes the relationship to Drupal core by:

• Adding compatibility with a new Drupal core major version while maintaining backward compatibility for all previously supported versions

It seems like this page's listing of core supported versions is out of date compared to statements on  https://www.drupal.org/about/core/policies/core-release-cycles/schedule → and  on  https://www.drupal.org/about/core/policies/core-release-cycles/release-p... → . Since it seems like  https://www.drupal.org/about/core/policies/core-release-cycles/schedule → is going to be the most frequently updated and canonical version, should this page ("Core supported versions") just reference a link to (or redirect to)  https://www.drupal.org/about/core/policies/core-release-cycles/schedule → ?

Since this issue is focused on how **contributed projects** should apply semantic versioning, I've added that to the issue title, and replaced "core support" with "core compatibility" to better reflect what I believe the focus of this issue is, i.e., whether to specify a major or minor release when adding/dropping compatibility with versions of Drupal core.

This works great for me! Requesting that the maintainers provide a new, Drupal 11-compatible release of this module, given that Drupal 11 has been released for over 6 months. Thanks for all you do!

+1 for a Drupal 11-compatible release. Thank you!

Thanks for reporting!

https://www.drupal.org/project/layout_builder_restrictions/releases/3.0.3 →

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

Thanks for adding the steps to reproduce, @rajab_natshah! The proposed new conditional check seems totally fine to accommodate edge cases in modules like Navigation and Layout Builder Retractions. Merged, with a new release on the way!

I can confirm that the patch in #6 resolves the issue.

Is there anything else the community can do to move forward on a Drupal 11-compatible release of this module? The work has been merged into the 3.0.x-dev branch. Thanks for this great module!

Thanks for the review, input, and communication about the timeline for this. I'll take a look at the suggestions & rework as I have time. I have no expectation/need for this to be fast-tracked for the upcoming release. Thanks!

Given that this feature explicitly does not want user interaction -- i.e., specific types of links should receive these data attributes consistently -- would an alternate, potentially simpler solution, be to create a text format filter for links in rich text fields, and/or to create a link formatter that extends Drupal core's formatter for link fields?

Creating a merge request for a new idea that I mentioned in the linked issue.

Proof of concept for the UI, not functional yet:

I've added the business logic for the functional bits. The test failure in the MR appears to be unrelated (it's also failing on 8.x-1.x)

This is ready for functional/code review.

At this point, I have no plans to merge this change.

I've updated the title & description in 📌 Document that URL redirects work from node/{nid} but not alias Needs work to indicate that the maintainers do not plan to support URL aliases. Work can continue there. This issue can probably be closed as "won't fix."

Based on the direction this is going in #22 and later, I think we can rename this issue to "Guide users who enter path aliases to use internal paths instead." I will also update the issue description to capture this.