quietone → created an issue.
Simply adding the issue template for coding standards.
I find the phrase, "in this sense", confusing but like the link. How about this;
Properties that are defined with property promotion should follow the property naming conventions in naming conventions → .
Actually add the "?" to the proposed resolution
In committer slack, awhile back, there it was said that Question 5 should be removed. Simply because committers wouldn't defer tests to a followup when a test is needed.
Why does this Installing Docker, DDEV & Drupal in WSL2 → say that Ubuntu needs to be installed first? In general, the whole section Installing Drupal with DDEV in WSL2 on Windows → is confusing to me. It has a lot of information that is not about installing.
Where to make issues for these pages?
Moved the page to the top of the menu.
Started updated the pages listed in the issue summary to direct to the new page. And updated the issue summary to keep track.
State that DDEV is the recommended solution.
https://www.drupal.org/project/documentation/issues/3398293 →
Direct to DDEV 'main' page. https://www.drupal.org/project/documentation/issues/3398293 →
Refer to main DDEV install page. Update drush link. Remove ref to drupalci. https://www.drupal.org/project/documentation/issues/3398293 →
Updated IS.
Would we then be allowing the following styles?
@deprecated in %deprecation-version% and is removed from %removal-version%. %extra-info%.
@see %cr-link%@deprecated in %deprecation-version% and is removed from %removal-major-version%. %extra-info%.
@see %cr-link%@deprecated in %deprecation-major-version% and is removed from %removal-major-version%. %extra-info%.
@see %cr-link%Restore the standard issue template and updated the proposed resolution to direct to #19.
@sourav_paul, Any contributor is welcome to review an issue. The Contributor Guide → has a step by step guide for how to review → and issue.
@gábor hojtsy, thanks those changes made a lot of sense.
I made a few grammar and spelling changes. I did find it a bit odd that the Project Lead was not listed within the team in the list of roles. So, I moved things around a bit so that all the roles in the leadership team are before other roles, and in the same order as the list of roles.
Being forced to split a large MR up just because it touches more than X number of files/lines/KB is just busy work to me, especially with these types of issues where the changes are mostly the same thing across every file.
On the other hand the policy is based on research that shows that the quality of a code review reduces as the change size increases. I do think Drupal should follow that, unless there is new research. And I know from my own experience as a reviewer that the research is correct.
Or is it more that this policy isn't a hard rule and we can decide on an issue-by-issue basis as a community whether something is too large?
There have been exceptions to the policy about the size of the change, such as when it is mostly boilerplate or a large change was automated. I think one of the recent OO hook issues was one, but I could be wrong. Coding standard issues would be another case where large patches were committed. So, yes there is some flexibility.
The language of the policy is already flexible in that it takes in consideration of the type of change and only suggests a 'good target size'. I do not see a need for a change.
On commit of the issue that originated this issue catch made the following comment 📌 Replace REQUEST_TIME in services Fixed which supports the existing policy
I'm going to go ahead and commit this despite #66 because I've already reviewed it in its current state once and it's looking good now. However the re-roll hell this has been in since December validates that it probably would have been easier in smaller chunks - e.g. splitting cache changes out to their own issue, or core/modules changes or both. It's always hard to balance between too many changes in one issue vs. too many issues for one change, and can be hard to tell when you start working on an issue how big the eventual MR is going to end up, so very tricky to figure out.
Where, the comment being referred to, comment #66, is the one where it was asked that the issue be split into smaller issues.
Tests were added in #940668: "Manage display" : Formatter change not reflected on settings → a few months after this was committed. I am updating the issue meta data and closing this as fixed.
To figure this out I would like to see the browser output from the test. However, the artifacts for the test-only changes test run does not include the browser output. Is this by design? Is there something that can be changed in the template so the browser output is saved?
To get the browser output I made an MR with just the test changes. And looking at all the pages of output hasn't given me an idea as to why the fail test passes in gitlab but fails locally.
Thanks.
Testing with that string does result in a phpcs warning
1815 | WARNING | The deprecation-version 'drupal:11' does not match the lower-case machine-name standard: drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] or project:n.n.n or
I added a Functional test that shows the error locally. But when I run the test-only changes it doesn't fail as expected. Gotta run.
In #7 @catch says that the string would change from 10.2.0 to 10.x. So, the existing string
drupal_common_theme() is deprecated in drupal:11.1.0 and is removed from drupal:12.0.0.
would become
drupal_common_theme() is deprecated in drupal:11.x and is removed from drupal:12.0.0..
That is confusing with 11.x being the name of the main development branch. That could be avoided by waiting until Drupal 12 is released, or we remove the suffixes so it would be drupal_common_theme() is deprecated in drupal:11 and is removed from drupal:12.0.0..
Instead of documenting this on https://www.drupal.org/about/core/policies/core-dependency-policies-and-... → , I added a link to list of issues tagged, 'approved dependency evaluation'. That way people can find the original issue easily enough and the Policy page won't need a change to remove the note later. I also added the link to the JavaScript dependency page.
Add link to issues tagged "approved dependency evaluation.
Add link to issues tagged 'approved dependency evaluation'
Added a draft CR. Will someone review it?
Yes, I read #135 and the other comments as well. Yes, this is a bug with a straightforward fix. That fix, however, has caused unexpected behavior as noted in #79, #101, #116, #121, #125 and #143.
And this fix does cause changes the UI, on a node/add form. The URL part of link field will be shown to the user as required. That is the change and yes it make sense. However, there is no information provided to let the use that the form can be submitted without completing the required field. That is why I asked for a usability review here 3 years ago and I still think one should be done.
All questions have been answered here and the basics are all in order. I updated credit. Leaving at RTBC.
I read the comments and the MR. All questions have been answered and this look fine. However, there is a question about a test, asked for in #9 and #27 but the tags, 'needs tests' was removed in #32 without comment. So, the question about a test needs to be answered.
I do think that this should have a test to prevent the re-introduction of a description on the form.
The deprecated version needs to updated to 11.2.0 instead of 11.0.0.
I left some comments in the MR. I also updated credit.
I read the comments and the MR and tested the change locally. I find that all questions are answered. I also updated credit.
And, I'd like to remind committers that screenshots should be available from the issue summary.
Leaving at RTBC
One remaining question here is #40. I'll leave that to another committer to respond to.
With the settings shown in the screenshot, before this change the link field is not required and after this change it is, even though the 'Required field' setting is not selected. I think that will cause confusion for admins and content editors, as it has done for people working on this issue. I think we find a way to resolve that here. For that getting an Usability review will help, so I am tagging and changing status.
Adding credit
This test added here failed locally so I restarted the test on gitlab and it failed there as well. https://git.drupalcode.org/issue/drupal-3476439/-/jobs/3610653
I reviewed this and the suggested change is an improvement.
I did a light review of the comment and I don't see any response to the suggestions by andypost in #24.
Since there are child issue, adding meta to the title.
The links mentioned in #87 are for lullabot.com, so d.o never stored that data.
Sites using Drupal 11 and MariaDB must use 10.6 or greater. This was set in 📌 Update INSTALL.txt and hook_requirements() etc. with remaining Drupal 11 platform requirements Fixed .
Therefor closing as outdated
@claudiu.cristea, thanks!
Yes, all is fine here.
I come back to report back that the 'significant changes' I have locally (#8) have been used to create several sibling issues that are size more in line with the core issue scope guidelines → . We now have duplicate work in those sibling issues and this one. Unfortunately, there are out of scope changes in this issue and the size of the MR is greater than what is ideal for reviewers and committers. So, I think we can preserve the work here by making this issue the last one to enable this sniff, but not in tests.
I am updating the issue accordingly.
linting has passed so changing to NR
quietone → created an issue.
And 'Please' is not used in the Drupal UI. I think this is a won't fix.
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
I don't see a bug here. The suggested change is removing information and patch is not implementing the proposed resolution.
@diptib, @digitalapicraft, I suggest you work on existing issues tagged 'novice'. And you can find more information about contributing in the Drupal Contributor Guide → .
@diptib, @digitalapicraft, Welcome to Drupal! Instead of making new issue I suggest you search for issue that are tagged Novice and work on those first. Also, consult the Drupal Contributor Guide → to find a task suited to your interests and skills.
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
@akulsaxena, thanks. The additions for the param type and return type are out of scope. The out of scope changes needs to be removed. This issue in only for adding the missing return comments so the sniff Drupal.Commenting.FunctionComment.Missing can be enabled. There are other sniffs for the param type, param comment, return type and return comment. There are issues for those which are children of #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard → .
quietone → created an issue.
quietone → created an issue.
quietone → created an issue.
This was RTBC before my questions and comments and none of the resulting changes effect the production code, so I am restoring the RTBC.
Credit from who made the release and who edited the google doc.
Added credit from the google doc history.
Adding credit from the doc history and who made the release
Adding credit for those who worked on the notes, using the google doc history.
An alpha was not released.
An alpha was not released.
RC! has been released, so changing title
I have updated credit. I am satisfied that my questions have been answered.
quietone → created an issue.
quietone → created an issue.
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
@akulsaxena, thank you for reviewing this. Did you also confirm that the comments I added correctly explain what the method is doing?
This passed linting so setting to needs review.
@chhavi.sharma, We unfortunately have a conflict. I did not see that you want to work on this and I have made significant progress locally. Can you push any changes you have made?
I am also unassigning because in Drupal core we prefer that people add a comment stating they are working on an issue. See Assigning ownership of a Drupal core issue → for the details.
quietone → created an issue.
I glanced at the MR and the size is well past that recommended in the Merge Request size → section of the scope guidelines. I suggest that this be split into smaller issues that are easier to review. Say, creating an MR for a group of changes that are implementing the same style of fix. I also noticed what could be inconsistent use of sprintf in the MR, that is, sometimes it is used and sometimes it is not. Can this be explained.
I agree that this needs more information. Also, changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
@eli-t, thanks for providing proof of the problem. Do you know if this happens on 11.x as well?
Ssince this is a long term strategy, it is not version specific, so I am changing to the main development branch.
Yes, we need more information to investigate this.
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.