Updated IS, made an MR with the suggestion.
quietone → made their first commit to this issue’s fork.
This part of enabling"Generic.CodeAnalysis.UselessOverridingMethod", which is in the Problem/Motivation section.
The scope is correct, it is fixing the instances where an unnecessary override was detected by the sniff. There is no change to phpcs.xml.dist because of remaining overrides, which are not unnecessary, will require discussion, and span the code base.
Adding a parent to help keep track of changes for this conversion.
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 → .
Rearrange with general first, Tag reference last, and alphabetical in between
Updating tags per the issue tag guidelines → .
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 → .
Updating tags per issue tag guidelines → . This is already in the 'menu system' component
Updating tags per the issue tag guidelines → .
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 → .
There are existing issues about improving the permissions for menus. It would help to add those as related and to ensure there are no duplicates.
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 → .
quietone → created an issue.
The question about quotes resulted in doing a bit of research. I commented on the MR and updated the IS with the result.
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 → .
Drupal 10 is in maintenance mode with a limited set of allow changes → .
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 → .
Drupal 10 is in maintenance mode with a limited set of allow changes → .
The remaining failures are expected because this issue is only for callbacks. The sibling issue needs to be committed first 📌 Fix 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard except for callbacks Active .
But I wanted to see if these are correct in content and meet the special standard for callbacks.
Since SafeMarkup was replaced by FormattableMarkup I think we can continue with this issue. There is already a child issue here for Kernel tests. Another one is needed for Functional tests.
As I understand #51, the follow up was to manage the number of changes. Since then, a separate issue has been created for Kernel test, which will help manage the scale of the changes. So, I don't think the tag is necessary any more.
I read the test \Drupal\Tests\system\Functional\Render\PlaceholderMessageTest::testMessagePlaceholder to improve the descriptions for the @return descriptions in RenderPlaceholderMessageTestController.php.
Linting has passed, so back to NR.
Did cleanup here and removed the out of scope changes and the '@return void'.
I think what is left is to improve the comments in core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php, some of which are incomplete sentences.
OK. It is now forbidden.
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 → .
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 → .
Nice!
Responded to longwave's feedback.
@joseph.olstad, One thing I like about this is the passion you have for improving Drupal. What I don't like is the disparaging comments in #58. Those are not aligned with the Drupal value of "treating each other with dignity and respect". Overall, this issue helped me understand your situation and the upgrade challenges you have to manage.
For reference, the value I referred to is in the Drupal Values & Principles → .
Tested again with @returns in
- includes/common.inc
- modules/dblog/dblog.module
- modules/migrate/src/Row.php
- modules/views/src/Plugin/views/field/MultiItemsFieldHandlerInterface.php
And PHPStan detected all of them with the same error message as in the previous comment.
It is still not clear where this should go. It could be added as a new page in the guide. PHPUnit in Drupal → . That would encourage the practice now and, if desired, an issue can be opened in Coding Standards to formally accept the practice.
I read through the issue to create a a bare minimum proposed text for the changes, which follows. What do you think?
Assertion messages
The default assertion message provided by PHPUnit is sufficient for most situations. The page explains when a custom assertion message can be added and the format of the message.
When to add a custom assertion message
- When the assertion in inside a loop.
- In a custom assertion method in a trait or base test class. A custom assertion method begins with
public function assert*();.- In a helper method.
Format of a custom assertion message
%subject% should %verb% %payload%Definitions
%subject%
The additional context information
%verb%
The action.
%payload%
The value the%subject% should be.
Example
foreach ($expected_steps as $element_id => $step_value) { foreach (['s', 'i', 'h', 'd', 'm', 'y'] as $sub_field) { $name = $element_id . '[' . $sub_field . ']'; $expected_step = $step_value[$sub_field] ?? 1; $input = $this->xpath("//form//input[@name='$name']"); $this->assertCount(1, $input, "Duration input $name should appear exactly once."); $actual_step = (integer) $input[0]->attributes()->{'step'}; $this->assertEquals($expected_step, $actual_step, "Duration input $name should have the correct step value."); } }where
{subject} == "Duration input $name" should == 'should' ;) {verb} == 'have' {payload} == 'the correct step value'
Tested on 11.x today
I changed the '@return' to '@returns' in \Drupal\views\Plugin\views\field\MultiItemsFieldHandlerInterface::getItems().
That resulted in this PHPStan error
------ ------------------------------------------------------------------------------------------------------------------
34 Method Drupal\views\Plugin\views\field\MultiItemsFieldHandlerInterface::getItems() has no return type specified.
🪪 missingType.return
------ ------------------------------------------------------------------------------------------------------------------
So, although not a phpcs rule the @returns should not be creeping back in the code base.
Shall we close this as outdated or make a change to phpcs.xml.dist to add '@returns' to the the list of 'ForbiddenAnnotations'?
Came across this because it is on 11.1.x instead of 11.x
@vidorado, thanks for thinking about what else needs to be done. However, issues to remove a particular deprecation are not required. When the time code to remove deprecated code, we can simply grep the code base.
Closing as works as designed since we have practices in place that will resolve this before Drupal 12.0.0
I too am not sure about setting it true by default. I am keen to find out what others think.
Added change record.
@martijn de wit, thanks for the screen shots. The screen shots should be available to reviewers in the issue summary and with an explanation of what they represent. They should at least indicate if they are before or after the diff was applied. And I do wonder if 14 screen shots are needed.
@developer-rocha, the MR is made against 11.x, so testing should be done there. Your verify the version by following the MR link in the issue summary.
I did forget that one. I left it because the @return is incorrect as well but never made back to change it.
@koustav_mondal, I think for now this needs to focus on splitting this issue into several smaller more manageable issues. To start can you move the changes in 'core/lib' to 📌 Replace http://example.com with https://example.com in core/ib Active . It is not the most exciting work but it does happen that we need to split an issue into smaller, more manageable pieces.
Given that this is changing nearly 200 files, it will likely need to be split further. I can check in on this in a few days.
quietone → created an issue.
How to deprecate → explains the steps needed to deprecate something as well as the format of the message. It is in the Core change policies → guide where other useful things about core development can be found. I didn't realize when I last commented here that this is a novice issue, and in that case, this information should have been in the issue summary. At least, in my opinion.
I edited the change record so that it contains only the necessary data. Anyone wanting to know more can follow the link to this issue. While it was well written we also want to be concise, simply stating what changed and any action needed to be taken. In this case there is no action, so it can be short.
This still needs code in the function to trigger a deprecation message. I have updated the issue summary.
Re-opened the MR so that others can use it to work on this issue.
@adwivedi008, research needs to be done on why the tests have lines of code that are not called.
Formatting
@bramdriesen, thanks for working on the security related pages. I am a bit confused though because all the old pages have been migrated when we have been only migrating what is current and deleting the rest. I now see Drupal 4 and 6 modules in the docs. Will this not confuse other readers as well?
Also, I went through another section.
I asked in Slack, #media, about why the test was specifically testing user #1. Two media maintainers, marcoscano and seanb responded saying that they saw no reason for it. seanb also said that they probably copied the test from somewhere else and then made it work for media.
So, I think it is OK to remove the specific assertions with user #1. Therefor restoring the RTBC.
quietone → made their first commit to this issue’s fork.
Another issue that needs a change record.
To answer #23, the Coding Standards committee itself does not have a charter. It is considered part of The Technical Working Group (TWG). The Job of the TWG is given in the TWG Charter, which states, "to ensure that the necessary policies, procedures, and standards exist to address technical concerns affecting the Drupal community". That would cover making a decision about which style of Markdown to use.
And my personal view is that using the GitLab style makes the most sense.
As @borisson_ points out what we have now is a mixture of the two styles and that, in itself, can be confusing. For some, when reading a file with both styles it raises questions, such why multiple spaces here but not there. And worse, which style do I use when editing the file.
I was just scanning core and the block of code in \Drupal\taxonomy\TermViewsData::getViewsData is a mixture of both styles. For me, it is an example of where allowing both styles reduces readability and that this proposal is a benefit.
How will the sniff know that the test is actually testing translations?
I updated the IS with proposed text.
As for removing the Drupal 6/7 outdated material from the standards docs, I think we can discuss that at a meeting.
@borisson_, I agree that the function parameters read better when on one line each.
Made changes per the feedback and since linting has passed, I am setting to NR.
Time to get some feedback on these changes.
quietone → created an issue.
Restoring status from #7
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 → .
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 → .
I reviewed the latest change and they look correct to me. I am always cautious about out of scope changes but there is logic in tidying up the references to Drupal 6/7/8 as part of explaining the new name.
Convert to MR
Convert to MR and updated it for the latest comments.
quietone → made their first commit to this issue’s fork.
Hi, the Drupal Core issue queue is not the ideal place for support requests. The 'support request' option is there for filing support issues for contributed modules and themes. There are several support options listed on our Support page → . This is 'Get Support' at the top of Drupal.org. There is also information about Drupal Slack → , at 'Get Support -> Drupal Slack' also at the top of Drupal.org. You may get better replies in one of those places.
quietone → created an issue.
In #15, it was suggested to scope issues by drupalCreateUser and drupalCreateNode. The first issue was completed but new violations have come back into the code base. Because of the I'd rather this is done by directory so the sniff can be enabled as fixes are made.
And for some facts, I enabled the sniff and phpcs reported 1,135 violations in 604 files. The longest line reported is 640, in var/www/html/core/lib/Drupal/Component/Utility/Xss.php.
quietone → created an issue.
quietone → created an issue.
The new text was not wrapped correctly at 80. I fixed that on commit using a PHPStorm plugin that does the wrapping for me.
Committed to 11.x and cherry-pick to 11.1.x
Thanks!
@annmarysruthy, thanks for doing the research on how the example changed. When doing that important work it helps the others working on the issue to add links to the previous issues and/or commits.
Using git I found that the change happened in 🐛 Example recipe isn't functional Active . And skimming that issue it looks like an oversight that this comment was not updated as well.
Yes, this reads better, thanks. There is just one more small thing, which I commented on in the MR. For background on that see 📌 [Meta] Use Install/Uninstall consistently for turning modules/themes on/off (not Enable) Fixed .
There has been no further discussion here. And @cilefen, has described the next actions here. So, I am closing this.
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 → . Also, Drupal 10.4 is in maintenance mode.
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 → .
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 → .