🇳🇿New Zealand @quietone

My didn't find usages either, https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...

@nikolay shapovalov, there are the Issue scope guidelines for Drupal core issues → and in there is a section about size and LOC, Merge Request size → . These are guidelines to ensure quality, not a set firm rule. The key part is that the "ideal merge request size varies with the type of fix". And like you say it is also about simplifying the review process. The MR, what ever the size or LOC change must me manageable for a reviewer in a reasonable amount of time, the research suggests 1 hour max at a time.

From #53, Trying for a better title.

I found one wee doc error in the MR and used the suggestion feature to fix that.

The proposed resolution should describe the change so that a reviewer/committers know what to expect in the MR.

I am among the many here, include the views maintainer, that are not able to reproduce this issue. The steps to reproduce is missing the step to install config_inspector and the link to view the report is a 404 for me. Is there other steps missing? I imported the view from #32 🐛 Missing schema for fields and filters on views with aggregation Needs work which is supposed to include the error and again, there are no errors with the config inspector.

Since this is important to Drupal CMS is there a tag to use? There is one for 'Drupal CMS release target' but I can't find an issue for the Drupal CMS release plan.

I've updated credit and leaving at RTBC due to the connection to Drupal CMS.

This was added in #2278383: Create an injectible service for drupal_set_message() → where the (optional) was valid for drupal_set_message. Should this be restoring the default value instead? I have asked in committer slack about this.

Yes, this looks correct. But I do think the deprecation messages should be adjusted. The sentence, 'There is no replacement' is better suited to method removal. See the comment in the MR

I read the IS, comments and the MR. I didn't find any unanswered questions or other work to do.

I read the IS, comments and the MR. I didn't find any unanswered questions.

Removing credit added earlier because 🐛 If computed view argument is empty the default value or action is not used Needs review was re-opened.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

This is failing commit-code-checks because the only file changed is a *.api file. I think there is an issue to fix that.

@matoeil, can you provide steps to reproduce, starting from install Drupal core?

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Restoring template. The issue template is used to track progress and various tasks on an issue.

Add credit for the commit and update issue meta

Adding credit for the commit and updating issue meta.

use plain language for signature change section

quietone → created an issue.

Committed 2408abb and pushed to 11.x.

Thanks!

Searching for the 2 functions gives 2 pages of results for each. But they are all in the simpletest module which is removed from Drupal 8+. One search was, https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...

I forget how often but api.drupal.org isn't updated daily.

Committed c6bc6eb and pushed to 11.x. Cherry picked to 11.1.x

Thanks!

Even for simple issues such as this one, leave the standard issue template in place. It can be unpredictable what is discovered while working on an issue and having the template already in place provides a place to keep track of anything that pops up.

I read the IS, comments and the MR. When removing code, even if it looks wrong, we should check the original issue to see if there is a reason this was done. I have not done that for this issue. The if block being removed was added in #1932652: Add image uploading to WYSIWYGs through editor.module → .

If this change is accepted there is a comment that would need to be changed. I left a comment in the MR.

Updated the IS to mention that this is relying on existing tests.

I read the IS and the comments. I didn't find any unanswered questions or other work to do. Updating credit.

Update for recent commits

More changes for recent commits.

Rebase with conflict in phpcs.xml.dist

Rebase with conflict in phpcs.xml.dist

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

did you mean 11.x? Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Add 11.1 to bugfix window

Committed 0be5673 and pushed to 11.x. Thanks!

#3490296

I read the IS, comments and the MR. This all looks good but I found some things that need work, so setting to NW. See the comments in the MR.

I am not sure about the removal of uri_callback: 'comment_uri',. I did read the comment from @berdir but it is used in entity.api.php and other files. I think that needs some investigation.

Yes, this removal looks correct.

I read the IS, comments and the MR. I didn't find any unanswered questions or other work to do.

I read the IS, comments and the MR. I didn't find any unanswered questions.

Is there a grep command to confirm that all instances have been changed?

And what about tests? For example. this changes the name in core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php but not in the associated test, core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php. That seems odd, at least to me. I think it would be better to change standardize them all. Is there a plan to do this in tests? I am not one to expand scope but it might make sense here.

Trying for a commit message

I read the IS, MR and the comments.

The suggestion by @larowlan in #18 has two parts and one has been implemented. The part to "add Breakpoint::hasMediaQuery instead of the empty, which is always a source of random issues" is not done and I see no comments about that.

Also tagging for a title update. The title should be a description of what is being fixed or improved. The title is used as the git commit message so it should be meaningful and concise. See List of issue fields → .

Needs to be rebased

Committed 798ad9e and pushed to 11.x. Thanks!

There are quite a few Functional test failures here in the Workspaces module.

I read the IS and the comments. I didn't find any unanswered questions or other work to do. Tweaked the change record some more.

I read the IS and the comments. I didn't find any unanswered questions or other work to do. Although noting that catch suggested using an alter hook.

This is changing the UI, so tagging Usability. This is set as a Task but is it not a bug?

I could not test the MR as it no longer applies.

I read the IS, MR and the comments . I didn't find any unanswered questions. I did leave a comment on the MR but before taking action it would be better to get the opinion of another committer to determine if this is the correct fix.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Restored the standard template, which we use to track progress and others tasks on an issue.

I tested this on Drupal 11.x, standard install, following the step in the issue summary. At step 3, there is no "dropdown menu icon in the upper right corner of the page". Is something missing? Is there custom code on the site?

@Benia, Thank you for the idea!

The proposal doesn't met the Criteria for evaluating proposed changes → . In this case, there is not demonstrated demand and support for the change.

smustgrave → credited quietone → .

The change in the 📌 Include #fragment in the link description text as an example Needs work is for the link module and not menu. And any change to documentation can be addressed over there. I think there isn't anything to fix here.

Rebased and added a test

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

I tested this on Drupal 11.x, standard install, today. I used the steps in the issue summary and was not able to reproduce the problem.

Is this something we still want to do? And if so, is the attached patch on the right track?

The issue summary is for fixing 'menu items' in the UI. I searched today and found no instances of 'menu items' in the UI.

(11.x)$ git grep -i 'menu item' | grep -v LICENSE.txt | grep -v migrate | grep -v ':\s*\*' | grep -v ':\s*\/\/' | grep -v ':\s*\/\*'
core/modules/menu_ui/tests/src/Functional/MenuUiTest.php:      $this->assertSession()->pageTextContains('Edit menu item');
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  title: 'Menu item title'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description parent'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  title: 'Menu item with a regular description'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description text'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description parent'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description child2'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description child3'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description child4'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description child4 overview'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description grand child2'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description grand child3'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description grand child4'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description parent'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description child'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description parent'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description child'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description parent'
core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml:  description: 'Menu item description child'
core/modules/system/tests/modules/menu_test/menu_test.routing.yml:    _title: 'Menu item with a regular description'
core/modules/system/tests/src/Functional/Menu/MenuRouterTest.php:    $this->assertSession()->pageTextContains('Menu item description text');
core/themes/claro/css/components/tabledrag.css:      using indentation (e.g. re-ordering menu items. */
core/themes/claro/css/components/tabledrag.pcss.css:      using indentation (e.g. re-ordering menu items. */

There are two instances in tests.

1. core/modules/menu_ui/tests/src/Functional/MenuUiTest.php: $this->assertSession()->pageTextContains('Edit menu item');
2. core/modules/system/tests/src/Functional/Menu/MenuRouterTest.php: $this->assertSession()->pageTextContains('Menu item description text');

The first one is in an if block in a test that is never reached. That method was committed years ago and may need to examined in another issue. The second is is testing strings from a test module, but the method, doTestDescriptionMenuItems, the assertion is in is never called.

I think this should be closed and another opened to look at the tests.

Converted to an MR

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

This has been waiting for steps to reproduce for nearly 2 years.

Since we need more information to move forward with this issue, I am setting the status to Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Un-assigning per Assigning ownership of a Drupal core issue → .

Thanks!

Based on #11 and #12 this has been fixed.

There hasn't been work on this in 9 years and there isn't a clear consensus here to go ahead with a change. I checked with @pameeela, who suggested to not make this change. I support that decision. Therefor, I am closing.

If you want to the change proposed in #32 re-open the issue and add a supporting comment.

Thanks.

Thank you for the idea!

The proposal doesn't met the Criteria for evaluating proposed changes → . In this case, there is not demonstrated demand and support for the change.

How does 📌 Locate drupalisms that might create confusion among users Active fit into this 'discovery phase'?

This seems about more than just 'menu' so changing component.
Un-assigning per Assigning ownership of a Drupal core issue → .

Added "stale-issue-cleanup" to the list of special tags with this description,

To track issues in the developing policy for closing stale issues, [Policy, no patch] closing older issues 🌱 [Policy, no patch] closing older issues Active

I think that because Boolean is the data type, not false or true. https://www.php.net/manual/en/language.types.php. And I do understand that this change helps.

There are currently 1064 instances of bool in and @return statement. What is the plan here so that we know that they are all fixed and how will we prevent future problems?

@shubhamporwal14@gmail.com, No, I am not saying this is fixed. Any changes to Drupal core are committed to our main development branch first. After that it may be backported to earlier versions, such as 10.4.x.

Also restoring the standard template.

Updated the remaining tasks regarding the question of tests and about testing images. All from the last 4 comments.

I read the IS, the comments and the MR. I didn't find any unanswered questions. I did add before/after screenshots to confirm the change and updated credit.

I read the IS and the comments. I didn't find any unanswered questions or other work to do. Reading the MR I found some coding standards to fix and I removed an unused variable from the test. These are minor, and I doubled checked, so I am leaving at RTBC, also updated credit a bit.

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

There hasn't been any discussion here for about 8 years and I am wondering if, in the meantime, that the multiple problems outlined here have been addressed?

Is there anything here that needs work?

This was already committed, and the follow up made. I am removing the tag. I also made a few tweaks to the followup.

I am closing as fixed now.

A few additions to the issue summary and this should have a title update before a commit. The title is current the same as the originating issue.

quietone → created an issue.

Tagging novice as the review is suitable for getting started with reviews.

quietone → created an issue.