🇺🇸United States @dcam

The following workaround code was adapted from #64. It corrects the problem of flagging all routes of the same name by also comparing the route parameters.

/**
 * Prepares variables for block__system_menu_block templates.
 *
 * Work around an issue where front page menu links are not set in the
 * active trail.
 *
 * Default template: block.html.twig.
 *
 * @see https://www.drupal.org/project/drupal/issues/1578832
 */
function my_theme_preprocess_block__system_menu_block(&$variables) {
  if (!\Drupal::service('path.matcher')->isFrontPage()) {
    return;
  }

  $homepage_path = \Drupal::config('system.site')->get('page.front');
  $homepage_url = \Drupal::service('path.validator')->getUrlIfValid($homepage_path);
  if (!$homepage_url instanceof Url) {
    return;
  }

  foreach ($variables['content']['#items'] as $key => $item) {
    /** @var \Drupal\Core\Url $url */
    $url = $item['url'];
    if (!($url instanceof Url) || !$url->isRouted()) {
      continue;
    }

    $is_front = $url->getRouteName() == '<front>';
    $is_configured_route = $url->getRouteName() == $homepage_url->getRouteName() &&
      $url->getRouteParameters() == $homepage_url->getRouteParameters();
    if ($is_front || $is_configured_route) {
      $variables['content']['#items'][$key]['in_active_trail'] = TRUE;
    }
  }
}

I updated the MR and dropped compatibility with unsupported versions of Core. I also removed the PHP requirement from the info.yml file. There's no reason to have it if we aren't using PHP functions from versions higher than what Core requires. Since we were previously requiring PHP 7.2 it isn't an issue.

Things have changed in the last year which I feel makes it necessary to drop the compatibility with unsupported versions of Drupal Core: we can no longer test against those versions now that Drupal CI is gone. With the switch to GitLab CI we're only testing against supported versions. Therefore, we can't make any guarantee of compatibility with old versions. They need to be dropped. I've done it with all of the other contrib modules that I maintain. I suggest that we do it here. It's pointless and potentially harmful to give the appearance of compatibility.

As it's already been said: people using old versions of core can just continue to use old versions of IEF.

Just an educated guess, but a possible of this is line 109 of StyleguideController:

$item['title'] = $this->t('@title', ['@title' => $item['title']]);

As you know some parts of the styleguide are dynamically generated and are dependent on your specific site's configuration, namely the image styles and layouts. If any of them are named '1', then that's how this could happen. You can test it by casting the item title to a string like this:

$item['title'] = $this->t('@title', ['@title' => (string) $item['title']]);

I verified the bug. The help page at /admin/help/styleguide crashes with an error about a Url object being passed instead of a string. The solution in the MR fixes the page and there are no more errors showing up in the site log. It's a simple bug with an easy fix. This is RTBC.

@larowlan the issue that I referenced in #8 also combined several JSON API test methods into one for performance reasons. This caused a problem for us because entities created by earlier tests remain in the database. Aggregator enforces uniqueness in feed titles and URLs. Due to this change, suddenly there was an existing feed in the DB with a title and URL that it wanted to create, causing failures.

I think the solution has to be deleting the existing feed before the other methods run. I queried for the feed entity rather than hard-coding the ID, just in case that changes someday due to further upstream edits.

The fails in Drupal\Tests\aggregator\Functional\Jsonapi\ItemTest were due to 📌 Speed up JSON:API ResourceTestBase Fixed . In that issue the method names were changed.

FeedTest will be harder to fix, I think.

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

dcam → created an issue.

dcam → created an issue.

+1 for this. I converted the patch in #4 to an MR. There's no need to give me credit for doing that.

My accessibility validator was also complaining about the <figcaption> outside of a <figure> element because it doesn't like invalid HTML. To be honest, this was going to be one of the things that I let go, but then I found this open issue. Applying the changes stopped that error. The patch looks good. The resulting HTML looks good. I don't know that there's much more to say about it. RTBC from me.

I'm reclassifying this as a bug report with normal priority since this causes an HTML validation error that may impact things like automatic scanning tools.

dcam → changed the visibility of the branch 3166502-showcase-figurefigcaption-together to hidden.

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

dcam → created an issue.

dcam → created an issue.

dcam → created an issue.

I'm adding another change to the description wrapper. Currently the descriptions are wrapped in a <p> tag.

The only time that the item descriptions are used is for image styles. But those descriptions are themed as an item list. The resulting <ul> are then wrapped by the <p>, which is invalid HTML. It would be better if the descriptions were wrapped in a <div> instead.

It was failing on D9 because I used the clean_unique_id Twig filter, which was introduced in D10.1. I initially changed it to clean_id before remembering that the point of putting it in was to guarantee the description id's uniqueness. So I took out the filter entirely. If this is committed after the D11 upgrade issue - which I hope will remove D9 compatibility - then you might consider reverting those two commits to restore the clean_unique_id filter.

I added the visibility check mentioned in #17 to the extra field. I think this is ready for review again.

I added some new assertions to check for the Redirects element. I repurposed the existing Node form test for that purpose. Since Users are content entities, they also get the extra field with this patch. So I simply checked for its presence on the users' own edit forms.

I converted the patch from #18 to an MR. There's no need to give me credit for doing that.

This needs tests. The existing functionality for the Node edit form has a test. It's possible that the test can be renamed and repurposed to check other entity types.

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

For anyone who needs a workaround, you can implement this in a custom theme or module:

/**
 * Implements hook_element_info_alter().
 */
function my_theme_element_info_alter(array &$info) {
  $info['checkboxes']['#process'][] = '_my_theme_process_options_element';
  $info['radios']['#process'][] = '_my_theme_process_options_element';
}

/**
 * Processes checkboxes and radios elements.
 *
 * Checkboxes and radios have an accessibility bug where the child option
 * elements inherit an empty aria-describedby attribute from the parent.
 * Because the child elements are fully rendered before the parent, this can't
 * be handled in preprocess functions.
 *
 * @param array $element
 *   An options form element.
 *
 * @return array
 *   The processed options form element.
 *
 * @see https://www.drupal.org/project/drupal/issues/2839344
 */
function _my_theme_process_options_element(array $element) {
  foreach ($element as $key => $value) {
    if (substr((string) $key, 0, 1) == '#') {
      continue;
    }
    if (!isset($value['#description']) && isset($value['#attributes']['aria-describedby'])) {
      unset($value['#attributes']['aria-describedby']);
      $element[$key] = $value;
    }
  }
  return $element;
}

The same thing can be accomplished by adding a process function to individual checkbox and radio buttons. I tested it. Doing that means you don't need to loop through all the keys in the parent element to find its children. But my preference was to target the checkboxes and radios since they're the ones that have the problem.

Oh, I get it now. You're asking because the term isn't rendered, it's written into other text. I'd have to give it some thought. I never use tokens that much, so I don't have advice off the top of my head.

The output of this module is a property of referencing entity, not of the referenced entity. So if this Views block displays the term data, then that's not going to work. We don't rewrite an entity's own URL and that's out-of-scope for this module. A new module would need to be written that could do that, though it's not a bad idea.

So here's what I've gathered from what you've said:

• This is a Views block.
• The block is being displayed on a product page.
• The view displays ingredient links. These other ingredients are similar to an ingredient that's in the product.
• The views has a header. That header has text into which a link to a specific ingredient is written. This is the link that needs to go to the faceted search.

Let me know if I understand any of that incorrectly.

Given what I said before about the referencing entity having the facet link, that means we would need to work our way back to the Product entity in order to get it. In general, I wouldn't expect that to be too difficult. All you need is a View that displays Product data, filtered by the context of the URL. Then you output the product's reference field linked to the facet and rewrite it to have your header text around it. At that point you can use it as an attachment view for other views.

The caveat to that is that you also need to filter that attachment view to a specific ingredient. You can pass other contexts to attachment views (I think), but I'm not sure how well that would work here. Also, I don't know how you give that original block that context in the first place. How does it know what your chosen ingredient is? That might help us figure out how an attachment view could work.

Sorry about that. It was a hastily-made in the web IDE. I should know better.

I don't know why the previous major test is failing. I don't have a D9 test environment on this machine because all our sites are on D10. I'll have to check it on my personal time later.

dcam → created an issue.

dcam → created an issue.

I removed the drupal/core requirement from the new composer.json file. Previously the documentation on adding a composer.json file specified that it's better to not define drupal/core as a dependency. But I just discovered that information was removed earlier this month, see https://www.drupal.org/node/2514612/revisions/view/13628499/13705720 → . Still, it's unnecessary and it can be a pain for someone who is updating modules.

I also removed compatibility for unsupported versions of Core, which has been my practice in the modules that I maintain.

I'm disappointed to find that the info about not requiring <code>drupal/core</code> was removed earlier this month.  I've referenced that information a number of times and was about to do it again.  <code>drupal/core</code> requirements in contrib modules can be really irritating, especially those that also require other dependencies.  If you're trying to update only that one thing and its dependencies, then suddenly all of core gets updated too.

I much prefer it when modules don't define a <code>drupal/core</code> requirement.  If a module is only compatible with specific versions of core, then in my opinion it's preferable to define that by specifying a <code>conflict</code> in the <code>composer.json</code> file instead of a requirement.

It might be a Drupalism to let the Composer facade do that work, but that doesn't mean it's a good idea.

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

I don't see why not. The ingredient is the taxonomy term, right? That doesn't sound like it would cause a problem to me. Is there a specific issue that you've encountered?

There was no further response. I'm going to assume this was a bug caused by hacking the code. Feel free to reopen it if I'm incorrect.

Hurray! Congratulations!

From my own comment in #3465577-18: Split sitemap_book config schema from sitemap schema → :

I'm guessing that a lot of the nullable keys were the result of all plugin settings being lumped into a single array. You had to do that so one plugin could leave all the other settings as empty.

While this config improvement is going on, any unnecessary nullable properties should be removed.

The Sitemap maintainers prefer to only accept merge requests that have passing automated tests. Automated tests ultimately benefit you. They ensure future changes will not break the functionality your site depends on.

If you need help writing tests, please ask us!

I just tightened it up a bit. And I removed "patches" since the patch workflow no longer functions. I wouldn't imply that it's ok for people to submit patches, which would add more work for you.

+1 for making an RC. You've done a great job closing the open bugs. Time to move this forward. TBH, I'd do it today.

I discovered today that our tests on D9 no longer work because \Drupal\Tests\field\Traits\EntityReferenceFieldCreationTrait doesn't exist in that branch...

If part of the code can't be run on D9, then the new version is officially incompatible with D9. A top module like this one likely has orgs running the automated tests as part of their deployment process. If any of them happen to be on D9, then those tests will fail. It's time to remove compatibility.

I reviewed the MR and it looks good to me. This is RTBC.

No problem. Glad I could help.

To prove my point I started to add a test for 8201 in the web IDE. Even with editing the fixture file to remove the path setting I was finished in a few minutes. But then I realized that this change may have further implications for the fixture since there's routing information in there. To be on the safe side, a new fixture would have to be generated for version 8200. So I backed off.

Bumping in case you don't get notified of the response in GitLab.

There you go. Please review it.

I had to add a new database fixture in order to test the config update. It should be suitable for testing any future config changes after schema version 8202.

This happened to me after I updated the module, but I hadn't run the database updates. Did you?

Actually, that may be a bad idea. What we're doing so far hasn't been very disruptive. But making something that can currently be NULL not nullable might have unintended consequences. I think it would be better to do that in a follow-up issue.

@mparker17 I'm guessing that a lot of the nullable keys were the result of all plugin settings being lumped into a single array. You had to do that so one plugin could leave all the other settings as empty. Do you think you could go through them in the MR and remove the key from all the settings that really should be filled out?

If you try to test this latest schema, then you'll get an error. You have to re-save your configuration first in order to insert the base_plugin key. This isn't ready yet because an update path is necessary.

Since you're waiting to put out an RC I can try to make the adjustments that I mentioned above before you commit this.

Easy fixes. I forgot to make the book schema dynamic and I still had a reference to that settings base schema in there.

I spun up a fresh dev site. I'm getting the same error in the config inspector. I'll try to diagnose the problem.

Gotta keep 'em separated.

Give this a shot. I developed this new schema in an active dev site and unfortunately problems with Search API's schema is completely breaking the Config Inspector. The WSoD is preventing me from doing that check. But the tests are passing here and on my local.

There's too much repetition. I had to copy the title setting to every plugin schema. I tried making a base schema for settings, but it didn't work. I have theories as to why, but knowing why wouldn't necessarily solve the problem. In the end, what we really need to do this properly is for the plugins to store their type. Then I think it should be possible to do something like:

# Base schema for sitemap plugins.
sitemap_plugin:
  type: mapping
  label: 'Plugin'
  mapping:
    enabled:
      type: boolean
      label: 'Enabled'
    weight:
      type: integer
      label: 'Weight'
    settings:
      type: sitemap_plugin_settings.[%parent.type]
    id:
      type: string
      label: 'Plugin ID'
    provider:
      type: string
      label: 'Provider name'
    type:
      type: string
      label: 'Plugin type'

sitemap_plugin_settings_base:
  type:
    mapping:
    label: 'Plugin settings'
    mapping:
      title:
        type: label
        label: 'Title'

sitemap_plugin_settings.frontpage
  type: sitemap_plugin_settings_base
  mapping:
    rss:
      type: path
      label: "RSS"
      nullable: true

...

But I wouldn't worry about that right now since you're gearing up for a release. If my MR works, then I'd commit it, ship it, and open a follow-up for fixing the repetition. Because doing that will require DB updates to add the new type key.

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

You asked for someone with technical writing experience to make suggestions, so I proofread it for you:

The Sitemap module displays one or more a human-readable lists of links on a page. A sitemap is a way for visitors to navigate your website via an overview of notable pages on the site. Sitemaps tend to be useful for sites with lots of lightly-organized content, for example, colleges and universities, governments, or organizations with many different units.

Features

• Configurable sitemap page path (default: /sitemap), title, and description.
• An extensible plugin API for displaying links to content based on Drupal entities. The module ships with plugins for these entity types:
  • Menus
  • Taxonomy vocabularies
  • Books (requires the sitemap_book submodule)

Installation

• Install as you would normally install a contributed Drupal module. →
• Configure the sitemap at /admin/config/search/sitemap.
• Set permissions for viewing and administering the sitemap at /admin/people/permissions/module/sitemap.

See the module's README.md file for detailed configuration instructions.

Extending modules

If you maintain any contributed modules that enhance the functionality of Sitemap, please add it to Sitemap's ecosystem by referencing it in the Ecosystem field on your project's page.

Similar projects

The Sitemap module provides human-readable sitemaps, not machine-readable sitemaps. If you want to generate machine-readable sitemaps, consider these options:

• XML sitemap →
• Simple XML sitemap →

Supporting this Module

The best way to support this module is to contribute to it! Contributions to add automated tests [7 issues] → are always welcome; and including tests in your bug reports, feature requests, etc. is a great way to fast-track your contributions! Automated tests allow the Sitemap maintainers to work faster, and they also help you, by detecting if a proposed change to Sitemap would break your site (i.e.: by detecting regressions)!

The Drupal Association (DA) → pays for the infrastructure that the Sitemap module maintainers use. Consider donating to the DA → to help keep Drupal.org → , the Drupal composer repository → , and automated tests running smoothly.

If you would like to hire a maintainer to add a feature to Sitemap that you want, please reach out to the maintainer through their contact form or their organization.

Notes:

• I mostly cut down on the amount of text. It was a lot to read.
• IMO, lists are good for people trying to ingest information quickly. Paragraphs are harder to parse.
• You can assume a certain level of knowledge already or that the information can be ignored. For instance, people don't need to be told about the Book module. If they need sitemaps for Books, then they'll already have Book installed. But if they don't need books, then you don't need to explain it.
• You're being overly generous by linking to Micro Sitemap, a module that has never been released and was never marked compatible with D10. I'd only link to serious projects or you waste the time of anyone who is looking for real extending modules. The same applies to Footer Sitemap (D7 only) and Menu Manipulator sitemap (D8 only, unmaintained, no releases).
• Noting that this module only does human-readable sitemaps is a good idea.
• Don't include the information about using Views and other modules to hack together sitemaps unless you want to field support questions about it.
• I left the Support section alone, but I would remove most of it except where you're implying that people can contact you about hiring you. But as for the rest: most serious contributors will know how to contribute already, but you could replace that paragraph with "Contact a maintainer or post an issue in the queue if you would like to know how you can help maintain the Sitemap module" or something similar. It's cool that you're promoting the DA, but to me it's just extra words to scroll past on what was already a really long page. It adds nothing to the information about the module, which is what this page is for.

I don't use Sitemap on versions that are EoL, but I am a fellow contrib module maintainer. For a long time I left compatibility for old versions of Drupal Core in my modules, even back to D8. It's only been in the last month while prepping for the D11 release that I finally stopped that practice.

My biggest concern at the time was whether I should drop support and then issue a major release or if it could be done in a minor release. I've seen both approaches in other modules. My strict reading of semver suggested that it would necessitate a new major release and that's why I'd always held off. So I did a search for info and found 🌱 Guidelines for semantic versioning and Drupal core support Needs review . I found myself agreeing with the side that says "drop support in a minor release." Now my thinking is that declaring your compatibility with Core isn't the same as declaring it as a dependency. So it's ok to say "We're no longer compatible with these versions" in a minor release. And as you said, we can't test against older versions, so you can't make a guarantee about compatibility anyway. With that in mind it could even be harmful to end users if you continue to declare that compatibility.

The end result for me was that I dropped EoL Core support for all my modules. I don't know if that perspective will assist you in making your decision, but I hope that I helped a little.

No problem. I'm glad I could help out.

In the end all I really did was simplify the code. I wasn't sure if this would break something, but all the tests passed on my local.

dcam → created an issue.

@larowlan any thoughts? This was prompted by the D11 update issue that's still trying to increase the core version constraint in the composer.json file.

Maybe I thought it was necessary due to the module's removal from core and that's why I left it in. But it never should have been necessary as long as the core version requirement in the info.yml file required D10.

dcam → created an issue.

I've seen the bot make multiple issues for multiple branches of the same project. That's what happened here.

@TR thank you for your hard work on this issue. Your perseverance and dedication to the community is deeply appreciated. I'm sorry that I couldn't help you with it. I've had other obligations demanding my free time.

I tested the 4.0.x branch on one of our sites today. The site loaded successfully without any WSoD. We only have a couple of rules in place on it: one for emailing admins when a new user is registered and another emailing after a content workflow transition. Both rules worked correctly after the update.

For years our theme used the recommendations above for doing region-specific menu theming.  I'm currently in the process of replacing that theme.  But we still need to render menus a specific way depending on the region in which they're placed.

This new theme utilizes single-directory components (SDC) all over the place.  The menus are no exception.  And those components contain all of the HTML I want wrapping the menus.  So when I re-evaluated this menu+region problem I realized something: the block prints the menu and the menu prints the component, but the block could print the component directly.  That would cut the menu template out of the process (that doesn't matter to me) and allow me to make theme suggestions for the block instead of trying to pass variables into the menu.  I came up with this:

MY_THEME.theme:

/**
 * Implements hook_theme_suggestions_HOOK_alter() for blocks.
 */
function MY_THEME_theme_suggestions_block_alter(array &$suggestions, array $variables) {
  // Menus are usually themed depending on the region in which they are
  // displayed. But they have no context about that region. The block does, so
  // add a suggestion to blocks and let the templates render the menu directly.
  if ($variables['elements']['#base_plugin_id'] == 'system_menu_block' && isset($variables['elements']['#id'])) {
    $block = Block::load($variables['elements']['#id']);

    if ($block && $block->getRegion()) {
      $suggestions[] = 'block__system_menu_block__region_' . $block->getRegion();
    }
  }
}

block--system-menu-block--region-REGION.html.twig:

{% embed 'MY_THEME:REGION_MENU_COMPONENT' with {
  'attributes': content['#attributes'],
  'items': content['#items'],
} %}{% endembed %}

I've only tested it a little, but so far it's working beautifully.  I can swap any menu into any supported region, even have multiples, and they automatically get themed appropriately.

I thought that I could use the more specific MY_THEME_theme_suggestions_block__system_menu_block_alter(), but for some reason the function wasn't being registered.  I had to move the code into the ...block_alter() hook.

Cool, I'm glad it's working for you!

That sounds like a perfect use case for the Facet URL formatter → . It was originally created for using as a token in Views. Change the formatter of your field to Facet URL, then rewrite the field's output to be <a href="{{ REPLACE_WITH_FIELD_ID }}">Compare this product</a>.

Let me know if that works for you.

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

dcam → created an issue.

dcam → created an issue.

Even if you're able to fix the problem for media, it will still be an issue for other fields. The Twig debug setting is just problematic for a view like this. That's why I included a note about it in the documentation. Personally, if I have to work with a timeline, then I just shut the debug setting off. You may want to consider implementing a custom template for the media field in your theme that uses Twig filters to strip the comments. Someone else with a similar problem posted a link to this Stack Overflow thread about it: https://drupal.stackexchange.com/questions/201275/possible-to-turn-off-t.... That might be simpler.

Actually, that error message doesn't make any sense. There is no argument 10 to EntityReferenceFacetFormatterBase::__construct(). There's no $module_handler variable in the entire module. Line 87 is a closing parenthesis and line 110 is a comment. Did you hack the code?

It looks like this is a module incompatibility problem. You're using a module called hook_event_dispatcher which overrides a service that ERFL expects. Try uninstalling the other module and see if that fixes the problem. If it works, then I will look into patching ERFL to fix the problem.

If I had wanted this issue to be closed, then I would have closed it.

While it isn't likely that we'll receive additional upstream changes from the update bot, it is possible. And if that happens, then I want the bot to be able to do its thing. That means leaving this issue open and tagged. I will close it sometime after D11 is released.

dcam → created an issue.

+1 for this. We've been working around this problem with Rules, but it isn't compatible with 10.3 right now. If openid_connect could do this for us, then we would have no need for Rules on most sites. I tested the patch on a site this morning.

I moved the patch into an MR since DrupalCI shut down in favor of GitLabCI. No need to give me credit.

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

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

Thanks for letting me know, @jrglasgow!

Closing since there was no further response.

dcam → created an issue.

I'll defer to @larowlan on that decision because I'm certain he's more aware of the implications than I am. But as I understand it, using $this->container can be fraught with problems. #2066993: Use \Drupal consistently in tests → has lingered in the Core queue for a long time, but the trend in that discussion was to standardize on \Drupal in tests because of the multiple issues that the container has caused. I'm inclined to follow that recommendation.

Many of the changes that are being found by the automated upgrade bots already have pending fixes in 📌 Fix gitlab and test next major Needs review . And the other issue solves the problems in more appropriate ways. For instance, it replaces all of the REQUEST_TIME instances, but without making calls to \Drupal inside of classes. My intention is to get that other issue committed first before proceeding with the upgrade. I'm just waiting for a final review (I hope) on that very big patch.

Yeah, I added nearly as many lines to the .module file as I moved to the libraries.yml. But I felt like the query string needed to be cached.

dcam → created an issue.