🇬🇧United Kingdom @oily

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 →

The pipeline is green. Ran test-only test and it fails.

Output: https://git.drupalcode.org/issue/drupal-3042423/-/jobs/5482721

Changing to 'Needs review'.

Applied IS template and tried to fit the existing IS into the different IS slots.

oily → changed the visibility of the branch 3555438-behaviours to hidden.

Fixed PHPSTAN and PHPCS. The unit test is failing.

oily → changed the visibility of the branch 3042423-hook-to-modify-oembed-resource-data to hidden.

oily → changed the visibility of the branch 3042423-add-a-hook to hidden.

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

Hid an old 9.x branch. The 11.x branch looks to follow a similar strategy, involves commits to the same 4x files except it also includes test coverage.

The test-only test is passing so test coverage needs work.

There is a merge conflict. Trying to fix.

oily → changed the visibility of the branch 2883521-expose-expire to hidden.

Have left code review.

Test coverage is now in place so removing 'Needs tests' tag.

The test-only test fails as it should:
https://git.drupalcode.org/issue/drupal-3025039/-/jobs/6753022

The pipeline is green. Setting to 'Needs review'.

What is missing? I notice that the issue affects migrations. Do we need migrations test coverage, also?

@smustgrave. Absolutely. Great idea. I just realised I never use the Review button but was not considering that it creates spam. It is a great Drupal feature so will start using it now.

Added stub functional test to the IFE module. Need to test the following scenarios: logins with bad username, bad password and bad username and password.

Assert that the status error message appears but that no IFE errors are output to the screen.

Have updated the IS. Applied the IS template.

Created new branch in. error (I did not realise that the existing branch/ MR had been attached to 11.x). Have hidden the bad branch.

oily → changed the visibility of the branch 3555258-tostring-method-is-11.x to hidden.

oily → changed the visibility of the branch 3555258-tostring-method-is to active.

oily → changed the visibility of the branch 3555258-tostring-method-is to hidden.

Edited the IS.

There were a lot of broken tests in the pipeline but did not look related. Re-ran pipeline and it is green.

The broken functional test looked unrelated. Re-ran the pipeline once or twice and now it is green.

Test coverage still required.

The spell check in pipeline is failing tried re-running but no good.

Output is:

CSpell: Files checked: 16410, Issues found: 0 in 0 files.
$ mv -f /build/core/package.json $CI_PROJECT_DIR/core/package.json
$ mv -f /build/core/yarn.lock $CI_PROJECT_DIR/core/yarn.lock
$ mv /build/core/node_modules $CI_PROJECT_DIR/core
mv: inter-device move failed: '/build/core/node_modules' to '/builds/issue/drupal-3515019/core/node_modules'; unable to remove target: Directory not empty

Has anyone seen this error before?

Last commit seems to have fixed the test coverage. The spell-check failure seems unrelated.

oily → changed the visibility of the branch 3515019-contact-form-throws to hidden.

Re: #14 That seems to be the least amount of code to reproduce and hopefully test the bug. If it is not sufficient then can create a simple form based on the multiple form in the examples project form api example. I have only tried adding the simple form alter to a custom module, installed the module on a D11 site, allowed un-logged-in users to register at /user/register then completed a registration at /user/register and submitted it. The error message appears.

Anyway the pipeline is failing and I cannot yet run the 'test-only' test. Seems to be unrelated test failures.

A programmatic way of reproducing the bug is to add a hook implementation to a module:

  /**
   * Implements hook_form_FORM_ID_alter() for the registration form.
   */
  #[Hook('form_user_register_form_alter')]
  public function formUserRegisterFormAlter(&$form, FormStateInterface $form_state): void {
    // Change the value of 'roles' to null
    unset($form['account']['roles']);
  }

So I have added that in a new test module to the user module. Will need a functional test that loads the test module in setup then submits registration form. Can then assert 200 http code.

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

Re: #7

Given this is going to affect so many tests (36 tests) because of the above, I wonder if the simpler thing to do is just to allow a $setting variable like $settings['disable_installer'] = TRUE; and if that would be easier to get in. Essentially I do not want to put lots of effort into something that will likely never get merged.

It looks like whatever fix is decided on it will definitely get merged. So should perhaps consider more involved fixes as it will be worth putting in the work. Though perhaps something quick and dirty that closes up the security hole like $settings['disable_installer'] = TRUE is good as a short-term measure while the code inside core is fixed to prevent this at source.

Re: #40 @hestenet I agree with some of your points. But I think you are omitting an important factor. There was certainly a condescending tone. But I think most people would consider his talk of 'Karma' as crossing into quite different territory! What next? 'You deserve to roast in hell'? When you introduce religion and guilt you can seriously offend people. The concept of 'Karma' by which I am supposed to be offending against the whole community is something that cannot be poo-pooed away.

Re: #38:

So here we have the situation that the some person pushes a commit on the basis of some quite old comment,

When I responded to the comment is was 3 days old. It is now 5 days old. I acted on it because there is a general 48 hours rule. You had 48 hours to deal with the comment. After 48 hours it is assumed that that person might not be returning for an indefinite period to continue their work on the issue. So I stood in to try to close one of the last comments and help everybody who has contributed to the issue.

obviously w/o reading and unserstanding the conversation at that point in time.

I doubt I understand it as well as you do. But 'understanding' is relative. There is a difference between trying to advance an issue and deliberately vandalising it. The whole ethos behind Agile project management and Drupal.org is a large Agile project is that everyone has a right to contribute without being blamed for mistakes they make. That is one of the pillars of Agile.

Please replace the word guesswork with a rejecting idiom for such behavior on the basis of Drupal cooperative conduct that feels more appropriate for you.

Defamation law does not work that way! I called x a 'murderer' but x should have substituted the word 'murderer' for something less defamatory that suited him better (by not doing so he defamed himself?!).

I ask you to be more thorough in the future, for the sake of respecting other people's lifetime.

@geek-merlin Your comments at #35 are not worth responding to.

Re: #35 I just added a choice constraint comme ça:

      constraints:
        Choice:
          - small
          - extrasmall

But I think a minor code refactor along with this instead would be better?:

      constraints:
        Choice:
          - default
          - small
          - extrasmall