🇬🇧United Kingdom @oily

Seems the MR was created by @joseph.olstad. @rcodina Not sure if it is necessary to create a new MR? Unless @joseph.olstad can resolve the comments?

@rcodina I have reviewed and approve your changes with my suggestions. I wanted to close the comments but do not have permission.

I think that the test function FilterTest::testRememberUserRoles() needs editing. A user needs to be created and the user needs to have a role added. Something like in this snippet found in core:

    $this->normalRole = $this->drupalCreateRole([]);
    $this->normalUser = $this->drupalCreateUser([
      'views_test_data test permission',
    ]);
    $this->normalUser->addRole($this->normalRole)->save();

There is existing test coverage inside the Functional test ExposedFormTest.php. ExposedFormTest::testRememberSelected() at the end of the file.

There is Functional test coverage: UserRememberRolesFilterSettingTest::testViewsPostUpdateBooleanFilterAcceptEmpty().

Also, Functional test coverage at: FilterTest::testRememberUserRoles().

It looks like the test coverage should go inside the Kernel test LocaleBuildTest.php.

Started code review. Change to the form field description.

PHPCS fix.

No credit despite at least #87. Seems I upset the drupal.org 'mafia' on this issue? : )

Would be helpful to get 'steps to reproduce' in the IS.

For testing I found this: core/tests/Drupal/KernelTests/Core/Datetime/Element/DatetimeFormElementTest.php. Lines 84 to 89 seem useful.

  /**
   * Checks we have no errors on form submit.
   *
   * @legacy-covers ::validateDatetime
   */
  public function testNoErrorMetOnFormSubmit(): void {
    // No error expected when form elements have no value.

As I understand it the error in this issue occurs when one of the datetime fields is left blank? And the error occurs on form submission?

oily → changed the visibility of the branch 3556851-deprecated-function-pregmatch to hidden.

Fixed merge conflict using inline editor. JS lint is still failing in pipeline.

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

Now the test-only test output is:

There was 1 error:
1) Drupal\Tests\filter\Unit\ProcessedTextTest::testPreRenderText with data set "empty text" (['', null, '', []], ['', null, '', [], ''])
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.
/builds/issue/drupal-3554602/core/lib/Drupal.php:178
/builds/issue/drupal-3554602/core/lib/Drupal.php:439
/builds/issue/drupal-3554602/core/modules/filter/src/Element/ProcessedText.php:163
/builds/issue/drupal-3554602/core/modules/filter/src/Element/ProcessedText.php:72
/builds/issue/drupal-3554602/core/modules/filter/tests/src/Unit/ProcessedTextTest.php:48
ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

Not sure if that error should be fixed? And a test failure should replace it..

For test coverage sounds like a functional test could work. This code block seems to be useful at lines 233 to 244 of the existing functional test CommentInterfaceTest.php:

    // Enabled comment form on node page.
    $this->setCommentForm(TRUE);

    // Submit comment through node form.
    $this->drupalLogin($this->webUser);
    $this->drupalGet('node/' . $this->node->id());
    $form_comment = $this->postComment(NULL, $this->randomMachineName(), $this->randomMachineName(), TRUE);
    $this->assertTrue($this->commentExists($form_comment), 'Form comment found.');

    // Disable comment form on node page.
    $this->drupalLogout();
    $this->setCommentForm(FALSE);

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

@smustgrave Sound good. I am wondering in this case though if this is a feature request? And might need updating of the IS extra fields..

oily → changed the visibility of the branch 3557674-serialize-property-returned to hidden.

Applied the IS template. Updated remaining tasks.

For the test coverage this form could be re-used:
core/modules/field_layout/tests/modules/field_layout_test/src/Form/EmbeddedForm.php

It could be re-used in a functional test in the inline_form_errors module.

The functional test could be a re-factor of:
core/modules/field_layout/tests/modules/field_layout_test/src/Form/EmbeddedForm.php

These ideas are based on the IS steps to reproduce.

Hi @mykola dolynskyi Not sure if you use the JS linter to fix JS?

Just in case you don't: If you have Node.js and Yarn installed on your local dev environment,

cd core && yarn install
Drupalize me states

Drupal core already ships with the configuration files ESLint uses to define coding standards for the project. This configuration is defined in a few files: in the root directory of your Drupal site you'll notice .eslintrc.json and .eslintignore files. .eslintrc.json tells ESLint to also include core/.eslintrc.json, which exends airbnb and plugin:prettier/recommended.

You should then be able to lint your JS files on the command line e.g.

core/node_modules/.bin/eslint themes/custom/friendly/js/friendly-greeting.js

Re: #10 started a code review.

There is a kernel test probably needs fixed. It looks like just one failure of an assert to deal with.

The functionaljavascript failure not sure how it is related?

However the test-only output is promising since it isolates the new test coverage and it fails:
https://git.drupalcode.org/issue/drupal-3557455/-/jobs/7278183

Looks like unit or kernel test coverage can be added given the steps to reproduce in the IS.

Here is the test-only coverage. It looks encouraging.
https://git.drupalcode.org/issue/drupal-3495006/-/jobs/7264917

But as suggested in #13 and in code comments, there may be possible for shrinking down the test coverage and enhancing it.

For functional test coverage to simulate #7 there is a views module functional test named ViewAjaxTest.php. It has two test modules as dependencies, 'test_ajax_view' and 'test_view'. These test views are configured inside core/modules/views/tests/modules/views_test_config/test_views.

Re: #12 I have commented the new test coverage. It seems very comprehensive and filling an existing gap in coverage but there is a lot of code. Is it all stricly necessary to prove this issue bug has been fixed. A follow-up could alternatively be created for any test coverage not stricly necessary.

#7 steps to reproduce seems to suggest that a functional test could be created and that might be sufficient. Possibly a functional test method within an existing functional test class?

Tried to create a new 11.x branch using Drupal.org instructions but too many conflicts that had to be resolved by editing. Abandoned attempt.

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

The pipeline is green and test-only output is as it should be:
https://git.drupalcode.org/issue/drupal-3083786/-/jobs/7249922

Re: #18 The IS summary seems to have been improved.

Ready for 'Needs Review'?

Fixed merge conflict.

oily → changed the visibility of the branch 10.2.x to hidden.

Added unit test. Needs work.

@smusgrave

3. not doesn't seem like a good config key

That is the key used for the existing exclude option in the numeric filter. So seems okay for that reason?

Added a schema entry.

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

Added a unit test. Needs work.

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

Fixed merge conflict. Now PHPSTAN and PHPCS errors and unit test failure.

oily → changed the visibility of the branch 3404451-link-field-bug to hidden.

oily → changed the visibility of the branch 10.1.x to hidden.

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

oily → changed the visibility of the branch 3083786-the-filter-convert to hidden.

I added a code suggestion.

Then edited the IS. Have added the screenshots with explanatory text from #110 to the IS.

This section of the IS probably needs replacing but best done by those more knowledgeable of the history of this issue than me:

Proposed resolution

Add label "Administrative side bar" to the navigation side bar. This aligns nicely with "Administrative top bar" label that we already have for the element that wraps the Top Bar.

We decided in this issue to explicitly not add aria labels for specific navigation blocks because this resulted in too many landmarks on the page - the one each for side and top bar are sufficient and a good balance.

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

@smustgrave There is one patch. I have examined the code and compared it to the 11.x code in the same method in the same file. I did not simply assume that it was right.

This issue is notable that in 7 years there has only been one patch submitted. The patch is by @claudiu.cristea who also created the issue. Certainly if one or more coders step forward to add new patches, or hopefully new MR's with their own approaches then the IS can be updated. In the meantime if the enhanced readability and the updating of the IS encourage contributors to create the necessary test coverage that can better determine whether the solution is good or not.

Re: #64, applied IS template and re-structured IS, some re-writing for clarity. I updated the 'Remaining tasks' according to #64.

2x broken functional tests in pipeline.
Test-only test appears good:
https://git.drupalcode.org/issue/drupal-3089151/-/jobs/7086430

Fixed PHPSTAN and PHPCS. Pipeline has failing unit test(s).

Hi @tuwebo, Sorry, I missed that. Was looking for green text : ) and missed the red!

Test-only test fails:

There was 1 failure:
1) Drupal\Tests\field\Kernel\KernelString\StringFormatterTest::testStringFormatter
Link containing href /entity_test_rev/1/latest found.
Failed asserting that an array has the key 0.
/builds/issue/drupal-2906455/core/tests/Drupal/KernelTests/AssertContentTrait.php:336
/builds/issue/drupal-2906455/core/modules/field/tests/src/Kernel/KernelString/StringFormatterTest.php:195
FAILURES!
Tests: 2, Assertions: 41, Failures: 1, PHPUnit Deprecations: 3.

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

Adding weight and type=hidden will still show the field in entity forms

Updated the IS using the IS template with minor edits and corrected the issue title according to #36.

The test-only test is failing as it should:

PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.3.22
Configuration: /builds/issue/drupal-2786763/core/phpunit.xml.dist
........F.                                                        10 / 10 (100%)
Time: 00:41.316, Memory: 8.00 MB
There was 1 failure:
1) Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStylePrivateWithConversion
Behat\Mink\Exception\ExpectationException: Current response header "Content-Type" is "image/png", but "image/jpeg" expected.
/builds/issue/drupal-2786763/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-2786763/vendor/behat/mink/src/WebAssert.php:180
/builds/issue/drupal-2786763/core/tests/Drupal/Tests/WebAssert.php:969
/builds/issue/drupal-2786763/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:228
/builds/issue/drupal-2786763/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:137
FAILURES!
Tests: 10, Assertions: 225, Failures: 1.

The test-only test outputs:

PHPUnit 11.5.42 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.14
Configuration: /builds/issue/drupal-3529674/core/phpunit.xml.dist
..F                                                                 3 / 3 (100%)
Time: 00:16.514, Memory: 8.00 MB
There was 1 failure:
1) Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest::testSwitchBetweenMultipleTabs
Failed asserting that false is true.
/builds/issue/drupal-3529674/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php:176
/builds/issue/drupal-3529674/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php:147
FAILURES!
Tests: 3, Assertions: 41, Failures: 1.
Exiting with EXIT_CODE=1

Test coverage seems fine.

@joachim Ah, good point. Updating accordingly.

Applied IS template and re-structured the IS accordingly.

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

This seems to be a duplicate of https://www.drupal.org/project/drupal/issues/2484411 →