#Needs Review Queue Initiative

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Ban Module was approved for removal in 📌 [Policy] Deprecate Ban module in Drupal 10 and move it to contrib for Drupal 11 Active .

This remains Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project → and the Extensions approved for removal → policies.

The deprecation work is in 🌱 Deprecate Ban module Active and the removal work in 📌 [12.x] [Meta] Tasks to remove Ban module Postponed .

Ban will be moved to a contributed project after the Drupal 12.x branch is open.

Moved patch from #2 to a MR

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

I am working on this issue.

I hide all patch files.
MR updated: move deprecation test to user module.

Have not reviewed but was previously tagged for tests and fixes should be in MRs vs patches

Moving this for NR.

I have checked this MR and found some conflicts, which I have addressed. The pipeline was failing due to an unknown word. After reviewing the existing test method, I added it to the cspell:ignore list, and the pipeline passed successfully. After that, I revalidated the test coverage, and it is working as expected. Let me know if you need any further adjustments!
Kindly review.

Failing test :

There was 1 failure:

1) Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testExtraFields
Behat\Mink\Exception\ExpectationException: An element matching css ".block-extra-field-blocknodebundle-with-section-fieldlayout-builder-test-empty" appears on this page, but it should not.

/var/www/html/vendor/behat/mink/src/WebAssert.php:888
/var/www/html/vendor/behat/mink/src/WebAssert.php:492
/var/www/html/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php:505

Moving back to Needs Review as the mentioned error in the file 3188136-nr-bot.txt → has been fixed by @Bhanu951.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I update deprecation test, because after changing currentUser to property promotion unit test failed.
Should we add type hinting for property or skip for now and add only at D12?

Using #markup bypasses field formatters, caching, and theming, leading to inflexible, inconsistent, and unmaintainable output. As per Berdir's comment, ✨ Respect form display weights in form preview Needs work overriding buildMultiple in the MessageViewBuilder and explicitly unsetting #sorted resolves the issue.
Just attaching the patch for now (Please validate if this is the right approach/place)

Thanks again for your feedback. I made changes, MR is ready for review.

Introduce return type hinting for create() is better at 🌱 [Meta] Implement strict typing in existing code Active .

Thanks for feedback. I will update MR.

1) Drupal\Tests\system\Functional\Form\ElementTest::testFormElements
Behat\Mink\Exception\ExpectationException: 0 elements matching xpath "//input[@type="button"]" found on the page, but should be 1.
/builds/issue/drupal-289240/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-289240/vendor/behat/mink/src/WebAssert.php:441
/builds/issue/drupal-289240/core/modules/system/tests/src/Functional/Form/ElementTest.php:161
/builds/issue/drupal-289240/core/modules/system/tests/src/Functional/Form/ElementTest.php:35
FAILURES!
Tests: 1, Assertions: 74, Failures: 1.
Exiting with EXIT_CODE=1

Shows the test coverage

Also resolved the threads because they appeared to all be addressed.

I did not find anything else pending and code looks fine to me.

Going to mark it.

Left some comments on the MR.

Hi @redseujac @alexpott, I've fixed the site slogan issue while using the Olivero theme. Now the site slogan is being shown for all the cases when the site slogan, site name and site logo have been enable on site-branding block configuration for the theme. I'm attaching the screenshots of the fixes for the issue. Please review the MR changes.

Thanks!

Feedback appears to be addressed.

Mark curent_user param as nullable.
MR is ready for review.

That's great I found this issue, because I wasn't sure if DI should be used here.
I will update patch, update message, add deprecation test and move from patch to MR approach.

Updated and created a merge request.

Only open thread is about cache hit ratio and I agree with @mxr576 that security trumps that. We can open a follow-up to further discuss those ramifications if need be.

All aboard the RTBC train, choo choo!

isNew() can be tricked, it most likely does not help PHPStan to narrow down the actual type of $node->id(), but I agree, let's follow the Drupal Way here -- funny, I wanted to use that originally, but I ditched the idea for some reasons, I hope not because tests started to fail :)

  public function isNew() {
    return !empty($this->enforceIsNew) || !$this->id();
  }

Really only see the isnew issue. The response to larowlans comment seems valid, and personally I agree with mrx on that.

If we can update that I'll rtbc again.

Yes they were random failures, restart make them disappear.

Test fails seem to be random ones, so if you can ping @bbrala or @larowlan for a final approval then it's RTBC in my book.

+1

Okay so I saw @bbrala's comment about isNew() now, but I'd still say we should change the $node->id() boolean checks to a version of isNew(). Reason being that if isNew() ever gets updated to be more strict, we immediately benefit from that change. If we keep manual ID to boolean conversions, then we may end up lagging behind on core changes.

MR looks good to me now for the parts that I can review confidently (so anything but jsonapi tests). Don't let my isNew() comment hold you back, although I think it'd be a nice change.

Test fails seem to be random ones, so if you can ping @bbrala for a final approval then it's RTBC in my book.

Setting back to needs work due to the failing coding standards issues picked up in the pipeline.

I've updated the issue summary, and created a merge request based on the patch from #71 as the patches have been deprecated.

It's been a while this issue is opened and the patches (re-rolls) apply and work succesfully, can this be merged into core now or, are there any other pressing issues stopping this patch merge?

Patch #71 works fine for me btw!

Patch from comment https://www.drupal.org/project/drupal/issues/3356667#comment-15753938 🐛 Error: Cannot read properties of undefined (reading 'settings')
 with dialog.position.js Needs work doesn't work for me on D10.2.8. Error is still there.

Resolved most of the Test case issues, few pipeline issues are unable to fix.