#Needs Review Queue Initiative

This needs a reroll on 11.x in an MR, the code has also changed a little bit (no jQuery) so the patch won't apply cleanly.

Might be good if someone can test this in Safari and see if it's still an issue?

mondrake reported in Slack that testing is failing on HEAD for PostgreSQL and SQLite.

I git bisected with PostgreSQL and found that this issue caused the problem.

I tried to reproduce the install/uninstall error in https://www.drupal.org/project/drupal/issues/2743175#comment-16228964 🐛 SqlContentEntityStorageSchema::getDedicatedTableSchema assumes that a fields property definitions matches the columns Needs work to check if this was a duplicate but I'm unable to reproduce the error anymore.

The spec says:

Note: Putting a property like "totalPages" in "meta" can be a convenient way to indicate to clients the total number of pages in a collection (as opposed to the "last" link, which simply gives the URI of the last page). However, all "meta" values are implementation-specific, so you can call this member whatever you like ("total", "count", etc.) or not use it at all.

Re: #11 I have added a @todo. To complete the todo may require the validation to be in place to test on the exact validation error text.

@ressa to answer your question not sure status report page would be needed to get the visibility it needs. But definitely should be documented somewhere, maybe the help topic?

Not sure that’s the test coverage we would need. The validation should be that these reserved words can’t be saved. So that’s what the tests should check I would assume.

I have added test coverage. Leaving the test coverage loose (assertNotContains(200)) as I do not think the validation is in place yet. Once it is, might be best to tighten the test to test for the validation string?

Thanks for fast feedback, it's really great that issues can maintain their momentum. I have changed the category to bug, and added some more tasks.

Thinking more about this, wouldn't it be great, if Drupal listed reserved paths somewhere, perhaps on the Status page? I have added a proposal in the Issue Summary, what do you think?

Wanted to double-check because removing "Extend" and "Collapse" was a big part of the original issue: Was the intent to keep "Extend" and "Collapse" in the names for stable9?

That is my understanding.

Any changes to the stable9 theme have the potential to break custom themes based on it, and perhaps cause automated tests to fail. We normally think in terms of the CSS in the theme, but I think it applies to JS, too.

I am removing the tag for an a11y review, thanks to @cwilcox808's comments.

I am setting the status to NW for additional test coverage (Comments #43, #44). I will try to get that done soon, but I have a 1-week vacation coming up. If I do not find time in the next two days, then it will be at least another week before I do it.

I've updated the comment in MR !4439. Could this please be reviewed? I wasn't able to resolve the comments, so if someone else could, that would be great!

Thanks.

Are you sure the meta key name has to be `total_count`?
According to the JSONAPI example https://jsonapi.org/examples/#pagination there is a reference to `totalPages`.

// Test an empty search via the block form, from the front page.
    $terms = ['keys' => ''];
    $this->drupalGet('');
    $this->submitForm($terms, 'Search');
    $this->assertSession()->statusCodeEquals(200);
    $this->assertSession()->statusMessageContains('Enter some keywords', 'error');

    // Confirm that the user is redirected to the search page, when form is
    // submitted empty.
    $this->assertEquals(
      $this->getUrl(),
      Url::fromRoute('search.view_' . $entity_id, [], ['query' => ['keys' => ''], 'absolute' => TRUE])->toString(),
      'Redirected to correct URL.'
    );

In above functional test, $this->getUrl() returns http://web/search/node?keys= for an empty search submission, while on the actual site the redirect is to http://web/search/node?keys (no =) with the change.

Faced the same problem with paragraphs. If a paragraph is unpublished in a translation, it should not be displayed at all.
Added a custom hook to achieve this:

/**
 * Implements hook_language_fallback_candidates_alter().
 *
 * Prevents falling back to another language if the paragraph’s translation
 * is unpublished in the current language.
 */
function MYMODULE_language_fallback_candidates_alter(array &$candidates, array $context) {
  $operation = $context['operation'];
  $langcode = $context['langcode'];
  $entity = $context['data'] ?? NULL;

  if (($operation == 'entity_upcast' || $operation == 'entity_view') && $entity instanceof ParagraphInterface) {
    if ($entity->hasTranslation($langcode)) {
      $translation = $entity->getTranslation($langcode);
      if (!$translation->isPublished()) {
        // Keep only the current language code to avoid empty candidate lists.
        $candidates = [$langcode];
      }
    }
  }
}

Added test coverage for adding route_name and route_parameters to the $options array in UrlGenerator::generateFromRoute() . Moving to NR.

catch → closed merge request !12973

I've gone ahead and backported this to 10.6.x and 10.5.x, was borderline on 10.5.x but there's literally no API change or behaviour change here it's just improving the performance of a very specific database query.

riyas_nr → changed the visibility of the branch 3040048--empty-query-parameters to hidden.

mlncn → changed the visibility of the branch 2906496-give-media-a to hidden.

Actually two assertions, to show that the name stays constant with the toggling (i.e., to show that we fixed that problem also).

I'd love to have an assertion for the (accessible) name of the button because the choice was very deliberate.

After a little discussion on Slack, a few days ago, with @nod_, we agreed that the stable9 theme should not be changed. It already has its own version of toolbar.icons.theme.css.

Wanted to double-check because removing "Extend" and "Collapse" was a big part of the original issue: Was the intent to keep "Extend" and "Collapse" in the names for stable9?

Needs tests and also dependency injection.

I've illustrated this quite vividly in the attached mp4 video with audio.

https://www.drupal.org/files/issues/2021-10-25/drupal_org_core_issue__31... →

Two runs in the video, one without the patch, the next with the patch.

Think this can actually be classified as a bug and may need some validation + test coverage. Since it results in a error message.

I've checked the latest changes on a test install and they look good to me.

Closed 🐛 External links cannot have empty query parameters Needs work as a duplicate.

In my case, I'm using the search_api_autocomplete to provide a list of search terms/content based on the user's input.
I was facing the same issue as described here. After applying the patch, the suggestions' options are now limited to the search box width.

Thanks for the patch.

I've backported the fix to 10.x by cherry-picking the 11.x commit.

@james.williams branch 11.x was merged into the issue's branch. Hooks were moved to new hook classes.

All tests passed:

dmitriid@drupal-core-web:/var/www/html$ ./vendor/bin/phpunit -c core core/modules/path
HTML output directory /var/www/html/web/sites/simpletest/browser_output is not a writable directory.

PHPUnit 11.5.22 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.21
Configuration: /var/www/html/core/phpunit.xml

....................................                              36 / 36 (100%)

Time: 01:04.175, Memory: 12.00 MB

OK, but there were issues!
Tests: 36, Assertions: 712, PHPUnit Deprecations: 60.


dmitriid@drupal-core-web:/var/www/html$ ./vendor/bin/phpunit -c core core/modules/path_alias
HTML output directory /var/www/html/web/sites/simpletest/browser_output is not a writable directory.

PHPUnit 11.5.22 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.21
Configuration: /var/www/html/core/phpunit.xml

S.............SS..SS..SS...............................           55 / 55 (100%)

Time: 00:53.023, Memory: 10.00 MB

OK, but there were issues!
Tests: 55, Assertions: 1795, PHPUnit Deprecations: 106, Skipped: 7.

Going to do a 10.x backport - I think this is worth it.

catch → closed merge request !9523

Opened 📌 Revisit latest revision / ::getActive() param conversion behaviour Active too to revisit the logic that causes this to be called so often (and also causes various other problems).

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Updated latest patch to make it apply to Drupal 11.2.3.
Added no tests in this update.

I opened a follow-up for 📌 Entity query in \Drupal\Core\Entity\ContentEntityStorageBase::getLatestRevisionId() should use a sort and limit Active

This came up in BSI random triage. #40 still applies.

I am updating the snippets in the issue summary to add the `visually-hidden` CSS class.

@flutterstack Thanks for your testing report. Sorry to hear you couldn't get the issue fork working, but the resolution for the current merge conflict with 11.x will need pushing to the MR with git somehow.

But regardless, it looks to me like your patch reverts the fix made in 🐛 Duplicate path alias when adding node translation Needs work ? We need to properly resolve the conflict rather than only accepting changes from this issue's branch please. Also, your patch removes some blank lines in core/modules/path/src/Plugin/Field/FieldType/PathItem.php (when compared to the current MR), but I think they are worth having for readability.

As for your question about the Entity Language Fallback module - it only implements two specific fallback operations, so doesn't act on the 'path_alias' one implemented here. (Whereas Language Hierarchy acts for all language fallback operations.) You could try adding the work done in ✨ Support getting the language fallbacks for path aliases Needs review if you have some specific need to use ELF anyway, though it lacks some other fundamental language fallback features (e.g. ✨ Support for t() Active , as well as needing a patch even for D11 support, which LH already provides). (Yes, I might be biased as Language Hierarchy's maintainer, but those are big factors!)