- last update
about 1 year ago 29,959 pass - Issue created by @mfb
- Status changed to Needs work
about 1 year ago 9:10pm 14 August 2023 - 🇫🇷France andypost
As I see media and user using notice
-\Drupal\media\MediaForm::save()
-\Drupal\media\MediaTypeForm::save()
-\Drupal\user\Form\UserCancelForm::submitForm()
-\Drupal\user\RegisterForm::save()
probably there's more and I bet it needs change record
- Status changed to Needs review
about 1 year ago 9:30pm 14 August 2023 - 🇺🇸United States mfb San Francisco
Added change record draft and release notes snippet. @andypost I'm not proposing further changes to user and media logging at this time, because we already updated those modules to log routine events as "info" and more significant events as "notice". Certainly if there is consensus though, more log level updates can be added to this issue.
- Status changed to RTBC
about 1 year ago 9:36pm 14 August 2023 - last update
about 1 year ago 29,961 pass - last update
about 1 year ago 30,047 pass - last update
about 1 year ago 30,056 pass - last update
about 1 year ago 30,056 pass - last update
about 1 year ago 30,058 pass - last update
about 1 year ago 30,060 pass - last update
about 1 year ago 30,060 pass - Status changed to Needs work
about 1 year ago 2:52am 30 August 2023 - 🇳🇿New Zealand quietone New Zealand
Thanks for continuing to improve the log levels!
I'm triaging RTBC issues → .
I read the IS and comments and was wondering if there was a Meta or some other discussion where these changes were agreed to. I found 📌 Demote routine content entity log entries from "notice" to "info" Fixed and read the IS and comments over there. In particular is comment #17 📌 Demote routine content entity log entries from "notice" to "info" Fixed where this task for all of core was scoped. And it turns out that that the patch over there was not modified in response. The same is true for this issue, it is using a different scope than recommended by a core committer.
There may be reasons for that change, and if there are, they should be commented to allow discussion. I also recommend making a Meta from comment #17 📌 Demote routine content entity log entries from "notice" to "info" Fixed so that it easier to follow the changes and to ensure that all the messages are changed. It would also allow others to pick up a piece of this task.
I read that xjm recommends only one change record. That is a good idea but only when the commits are to the same branch. Also, when there is a new CR, like in this issue, it should have a different name than the previous one to avoid confusion. I would go so far as to say the CR from the other issue should have a title change to reflect which kinds of messages are demoted. But I haven't checked to see if that would make the title too long.
Because of the scoping I am setting this back to NW.
- Status changed to Needs review
about 1 year ago 3:44am 30 August 2023 - 🇺🇸United States mfb San Francisco
@quietone I'm unclear what scope you are proposing. For example, we could split this issue into 5 issues: One for email action, one for comment posting and deleting, one for contact emails, one for search, and one for taxonomy create and update. Maybe that's too much issue overhead, or does that sound reasonable?
- 🇳🇿New Zealand quietone New Zealand
@mfb, I am referring to the scoping set out by xjm in comment 17 of the current parent issue, #3255699-17: Demote routine content entity log entries from "notice" to "info" → .
- 🇺🇸United States mfb San Francisco
@quietone ok, that doesn't mention taxonomy so I kinda have to extrapolate. How about splitting this into 3 issues: one for email action and contact emails, one for comment post/delete and taxonomy create/update (since those are both content entities), and one for search? My take would be that this is not worth splitting into multiple issues, but *shrug* whatever's easier.
- last update
about 1 year ago 30,100 pass - 🇺🇸United States mfb San Francisco
Split off two child issues: 📌 Demote email log events from "notice" to "info" Closed: duplicate and 📌 Demote search log events from "notice" to "info" Closed: duplicate and rerolled with just the content entity related hunks.
- Status changed to RTBC
about 1 year ago 2:39pm 31 August 2023 - last update
about 1 year ago 30,135 pass 53:01 49:23 Running- last update
about 1 year ago 30,136 pass - last update
12 months ago 30,146 pass - last update
12 months ago 30,146 pass - last update
12 months ago 30,148 pass - last update
12 months ago 30,154 pass - last update
12 months ago 30,161 pass - last update
12 months ago 30,161 pass - last update
12 months ago 30,168 pass - last update
12 months ago 30,166 pass, 1 fail The last submitted patch, 9: 3381078.patch, failed testing. View results →
- last update
12 months ago 30,193 pass, 2 fail The last submitted patch, 9: 3381078.patch, failed testing. View results →
- last update
12 months ago 30,205 pass - last update
12 months ago 30,363 pass - last update
12 months ago 30,365 pass - Status changed to Needs work
11 months ago 1:19am 30 September 2023 - 🇺🇸United States xjm
This looks great. However, instead of creating a new CR, can we just attach https://www.drupal.org/node/3381101 → and update it to include the changes from this issue? Thanks!
- 🇺🇸United States smustgrave
@xjm may be late but I don't follow, the attached link is to this CR. So not sure whats meant by
can we just attach https://www.drupal.org/node/3381101 → and update it to include the changes from this issu
- Status changed to RTBC
11 months ago 2:08am 30 September 2023 - 🇺🇸United States xjm
lol my bad. Don't mix sleep medication and the RTBC queue, folks.
- Status changed to Needs work
11 months ago 2:09am 30 September 2023 - 🇺🇸United States xjm
Oh that said I would like other issues to be added to the scope. All the "Demote notice..." issues should be merged into one unless there are like 50+ such reductions.
- Status changed to Needs review
11 months ago 8:26pm 30 September 2023 - 🇺🇸United States mfb San Francisco
Lol ok restoring the initial patch, issue summary and title
- Status changed to RTBC
11 months ago 11:47pm 30 September 2023 - 🇺🇸United States smustgrave
Thanks @mfb for closing those others out.
- last update
11 months ago 30,360 pass - 🇺🇸United States xjm
Ahhh I see what happened. The key phrase from #3255699-17: Demote routine content entity log entries from "notice" to "info" → is:
If we need to split it up
In this case, its size and change pattern did not merit being split up. It's truly impressive that I managed to write a 3000-word comment and still did not manage to get the key point across. 😅 It is explained in the scope doc I linked, but in reality it's a lot of info to process.
The key considerations are the size of the changeset in added plus removed lines, and whether the changes can be scanned and reviewed without context. If you need to look beyond the changed lines to get the context, smaller scopes are better.
If something is scannable and only requires a
git diff --color-words
to validate, and the total scope of potential changes is already documented along the lines of my comment from the other issue, it's safe to have at least 100-200 total lines to review (50 to 100 one-line changes).The only time we would split up a "no context required color-words issue" would be if it were more than 300-400 lines (150-200 one-line changes), or if parts were patch-eligible and other parts were minor-only, or if there were interdependencies or merge conflicts with other issues.
If you want to learn more about why these details of scoping are important, the video of my DDD Vienna session on code review explains the psychology behind all this. 🙂
- last update
11 months ago 30,371 pass 53:00 48:50 Running- last update
11 months ago 30,377 pass - last update
11 months ago 30,382 pass - last update
11 months ago 30,392 pass - last update
11 months ago 30,397 pass - last update
11 months ago 30,397 pass - last update
11 months ago 30,413 pass - last update
11 months ago 30,417 pass - last update
11 months ago 30,426 pass - last update
11 months ago 30,427 pass - last update
11 months ago 30,437 pass - last update
11 months ago 30,441 pass - last update
10 months ago 30,464 pass - last update
10 months ago 30,480 pass - last update
10 months ago 30,485 pass - last update
10 months ago 30,486 pass - last update
10 months ago 30,488 pass - last update
10 months ago 30,511 pass - 🇺🇸United States xjm
Ohh, on the CR front, I meant adding them to the previous CR in https://www.drupal.org/node/3259469/edit → . I updated that CR to include this issue and its changes, and deleted the new draft.
Crediting @mfb for patch work, @smustgrave for helping re: the CRs, and @quietone and @andypost for asking scope questions.
Is there a separate followup for the media events @andypost suggested? If exists and is RTBC-ish, it can also be included here. If not, we should file it. Thanks!
Promoting to normal; "minor" should only be used for nitpicks. This change improves the UX of watchdog, so it's not a nothing-change.
- Status changed to Needs work
10 months ago 9:49pm 9 November 2023 - 🇺🇸United States xjm
Oh, and NW until the followup is either filed or merged into this issue.
- Status changed to Needs review
10 months ago 10:35pm 9 November 2023 - 🇺🇸United States mfb San Francisco
As I mentioned in #3 I am not proposing further changes to Media logging
- Status changed to RTBC
10 months ago 11:28pm 9 November 2023 - Status changed to Fixed
10 months ago 3:19pm 10 November 2023 - 🇺🇸United States xjm
OK, sounds good!
Committed to 11.x and cherry-picked to 10.2.x. Since this is a behavior change for things in the admin UI, I was on the fence about backporting it to a patch release. I checked the previous issue and it was not backported past 10.1.x, so I followed the same pattern here.
Thanks everyone!
- Status changed to Needs work
10 months ago 12:07am 11 November 2023 The Needs Review Queue Bot → tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
- Status changed to Fixed
10 months ago 12:08am 11 November 2023 Automatically closed - issue fixed for 2 weeks with no activity.