Account created on 20 June 2006, about 18 years ago
  • Drupal Core Committer at AgileanaΒ  …
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States xjm

Very nice work @Feuerwagen! Setting NR for review.

πŸ‡ΊπŸ‡ΈUnited States xjm

Oh FWIW regarding:

Was expected a 1 to 1 for additions to subtractions but see some phpstan things were fixed in the process. Not super clear how adding void to doTestGrouping() fixed those but the checks are all green so won't think too much into it.

If the only thing that's not adding : void is deleting lines from our PHPStan baseline, that's better than good. If there are other changes to actual code, though, that would be something to keep an eye on and document.

πŸ‡ΊπŸ‡ΈUnited States xjm

Oh also: We can discuss what to do with the leftover methods once the easy/non-disruptive ones are committed. E.g., breaking ResourceTestBase or MigrateTestBase causes pain for a small handful of contributors who are already very active enough to address it quickly, but breaking BTB methods themselves is probably not something we should do in 11.1. So that's the only example where the RC does make a difference, I think. But that's a few lines out of thousands here that we can clean up from the parent(s) once the bulk of the work is committed.

πŸ‡ΊπŸ‡ΈUnited States xjm

I just mentioned to @longwave that IMO this actually isn't tied to RC at all -- we would want to backport as much of this as possible to 10.3 to cut down on merge conflicts. I apologize for not giving better/clearer advice to start! Hopefully it's just a matter of tweaking a filename matching pattern or two to exclude things called *Base.php or *Trait.php.

πŸ‡ΊπŸ‡ΈUnited States xjm

Also things with lockfile changes are never good candidates for the friendly bot as they break constantly, and it's not worth rerolling something forever without other progress. So tagging so the bot leaves it alone. Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

Do a better job explaining the dep evaluation and link a couple decent examples (that helped me make RM decisions about previous dependencies).

πŸ‡ΊπŸ‡ΈUnited States xjm

#2 is still not addressed; this issue could be a whole lot of wasted effort if the dependency doesn't pass the dependency evaluation. I recommend ceasing rerolling/rebasing/etc. and instead completing the dependency evaluation, then tagging it "Needs release manager review" and "Needs frontend framework manager review".

πŸ‡ΊπŸ‡ΈUnited States xjm

Sorry for noise; somehow either commenting about the CR or closing the MR ended up crediting me for this issue which is not correct. πŸ˜‚ Fixed now.

πŸ‡ΊπŸ‡ΈUnited States xjm

@acbramley I assume it was a typo. In this case, a release manager's decision in #30 and #32 that this is too disruptive for a maintenance minor is what stops it from being backported to 10.4.

11.1 and 10.4 ship at the same time, but 10.4 is supposed to have very limited changes per the maintenance minor docs β†’ . But this is the first time we've ever had a maintenance minor and so the committers haven't established a detailed policy yet; we'll learn as we go, just like we did each previous time we improved the release cycle. :)

πŸ‡ΊπŸ‡ΈUnited States xjm

It's tagged -- thanks everyone!

πŸ‡ΊπŸ‡ΈUnited States xjm

And yes the "hardcode dates" one is more of a should-have; it's only must-have for 10.4 (but the 10.4 meta isn't a thing on its own ATM due to it being a maintenance minor).

πŸ‡ΊπŸ‡ΈUnited States xjm

Moving some outstanding thingies to the 11.0.0 meta along with things we should check for again prior to release.

πŸ‡ΊπŸ‡ΈUnited States xjm

I seem to have invented a patch release in my head.

πŸ‡ΊπŸ‡ΈUnited States xjm

Discussed with @longwave and @alexpott. It's weird to have a behavior change of any kind in 10.4 and 11.0 but not 10.3 -- 10.4 is supposed to be a maintenance minor that limits disruptive bugfixes, and 11.0 is almost-RC-ish as of the week this was committed. We discussed and agreed the disruption is minimal enough to backport to 10.3.x, which resolves potential 10.3/11.0 parity issues with this API. There is a CR, so the few projects that might be affected can get the info they need.

If this does cause more serious issues for contrib or sites, please open a followup issue linking this one and tag it "Contributed project blocker" if appropriate. Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

Published the CR.

πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States xjm

For #17 (manual testing with multilingual/localization awareness).

(We might later want feedback from date or localization maintainers to make sure we're using the APIs correctly since it was always sort of a hack and so far this changes the specific format in the hack, but we might also be able to figure that out just from the testing.)

πŸ‡ΊπŸ‡ΈUnited States xjm

Looks like my feedback from the previous issue did not make it into the IS here.

As I pointed out on the issue that this was split off from, the original date formats were chosen to be reasonably understandable without having to get international localization etc. involved for a status report message, but then trying to mitigate the readability again afterward with the month abbreviation, and I think both might have been wrong. (The original convo went something like: "We shouldn't use an American date format because it could confuse people internationally" "Well, year-first is unambiguous" "Ahh but hmm numbers only are harder to read/scan/understand" etc,

For this issue's change: Why is "2023-Nov-30" okay but not "2023-Nov"?

We should test it in (a) a language that uses the Gregorian calendar but not US date formats and (b) a language that doesn't necessarily use the Gregorian calendar (Arabic or Hindi maybe), and consider a more complete fix.

πŸ‡ΊπŸ‡ΈUnited States xjm

Updated the release note to explain who is affected.

πŸ‡ΊπŸ‡ΈUnited States xjm

No hyphen in "coincide".

πŸ‡ΊπŸ‡ΈUnited States xjm

Fix confusing language that made it sound like security releases were likely to happen outside the windows, and fixed overuse of italics.

πŸ‡ΊπŸ‡ΈUnited States xjm

Formatting issues and missing branch from security coverage info (did the page get an update from a non-RM? need to check).

πŸ‡ΊπŸ‡ΈUnited States xjm

Fix missing info about 10.3's release and 10.2's EOL date.

πŸ‡ΊπŸ‡ΈUnited States xjm

I added the EOL date for 10.3.x, which had gotten deleted somehow. Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

@breezeweb For currently supported versions, the EOL dates are all documented. The Drupal 10 series does not have an EOL date yet because it will not be EOL until at least the release of Drupal 12 or possibly even after. When an EOL date is chosen, it will be documented on the linked page.

See also:  https://www.drupal.org/project/drupal/issues/3399348 πŸ“Œ Template for Core release cycles diagram RTBC

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

Adding credits.

πŸ‡ΊπŸ‡ΈUnited States xjm

Updated for the related issue @quietone separated out.

πŸ‡ΊπŸ‡ΈUnited States xjm

@Chandansha, this is a "Plan" issue, which means we do not create merge requests against it. Instead, we make child issues for the individual tasks with different kinds of fixes according to our scope guidelines. Please don't create merge requests for a single word; furthermore, I would not recommend working on the child issues until we've fixed the plan for them. Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

@quietone raised that this would exacerbate an existing problem we have, in that our list of active subsystem maintainers is not well-maintained and we have a lot of people who have elevated permissions for Drupal core but who are not actually active as maintainers.

This is probably not blocking as it is just an extension of a problem we already have, but it does mean that it would be good to make sure that the granting and revocation of this access is automated, if possible (maybe tied to the "maintain issues" permission on the project node).

πŸ‡ΊπŸ‡ΈUnited States xjm

Saving some credits.

πŸ‡ΊπŸ‡ΈUnited States xjm

Thanks for your patience on this issue.

I discussed this issue at Dev Davs with @catch and @daffie following discussion with @acbramley in Slack.

The new performance is great; however, the downside of the approach currently being used is that it won't work on no-SQL databases. We discussed this with @daffie and he will propose a related issue that will allow core updates to better support an SQL case and a no-SQL case.

We also discussed the proposal to set it to 0 as @Berdir suggested. We're concerned that this could have unintended side effects in edgecases or specific site configurations.

If the data validation allows it to be set to NULL, that would be ideal. Then we can add code that happens on entity save to set a given block's author based on the first revision, so that it happens as-needed rather than in a big batch. So that's the approach we recommend (so long as it is feasible).

Meanwhile, this issue has a data model improvement that should have a change record and maybe a release note. (We'll decide after the final approach whether to use the release note, but having a good release note will help us think about the impacts on site owners, code, etc. as well as edgecases.)

Leaving "Needs release manager review" pending the new approach. Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

The existing date formats were chosen for being reasonably understandable internationally, rather than having to get the date formatter involved for a status report message.

If we want to propose changing that too, I would suggest a separate issue.

πŸ‡ΊπŸ‡ΈUnited States xjm

I was leaving it open on purpose to remind myself to look at #28 and #29 once more info was provided.

πŸ‡ΊπŸ‡ΈUnited States xjm

@nicxvan, correct, only changes that are disruptive should go in the release notes. The release notes should describe the disruption the site owners or module developers need to address and how.

If it's just an improvement or feature, it should be tagged for the highlights instead.

I don't have time at the moment to look over these three issues, but if they are disruptive, then the release notes should be updated to describe what the disruption is, who is affected, and what they need to do to fix it, with a link to the change record. For more information and examples, see the release note snippet documentation β†’ .

Leaving NR to check back on them.

πŸ‡ΊπŸ‡ΈUnited States xjm

I think this issue might be a wontfix:

  • Having a new required field on every single Drupal site, especially one that does not affect the production user experience, seems like it is fixing an edgecase at the expense of the evaluator and site owner experience for all sites.
  • Adding additional form elements always inevitably causes a usability regression, so we should make sure the usability benefit of adding the element outweighs that regression.
  • It is also relevant for the product manager role because it affects the evaluator experience of Drupal. Adding an additional thing that must be configured (at site install maybe even?) is a non-trivial increase in the amount of configuration necessary to get up and running.

At a minimum, I think the field should be optional rather than required, and have a sensible default value if the user doesn't provide one. Furthermore, though, this could be addressed with a contrib module and might not even be appropriate for core.

If folks do still feel this belongs in core, then at a minimum, the field should be made optional. Then, that could be submitted for usability review (tagged "Needs usability review") once that change is made. Furthermore, even if the usability team gives the addition a +1 (benefit outweighing negative UX impact), this would still require product manager signoff because it affects the evaluator experience of Drupal.

Also, there is not actually a merge request here ATM, so we would need that too before proceed.

My personal recommendation is actually to mark this wontfix and support it via contrib, but it's outside the scope of my role as a release manager to make a final decision on that myself.

πŸ‡ΊπŸ‡ΈUnited States xjm

Though the thing with the entity publishing status might be a different, separate issue also.

πŸ‡ΊπŸ‡ΈUnited States xjm

Per @larowlan ✨ Add view tab + standard template for block content Needs work would resolve this, so maybe this should be closed dupe and its STR incorporated into that issue's summary and test coverage. Postponing for now until I can take a closer look at the other.

πŸ‡ΊπŸ‡ΈUnited States xjm

I just filed πŸ› Block library view links to the edit page for each content block even when the user does not have edit access Active which may be a duplicate. This issue has wider scope, but would resolve that.

May check back later to incorporate my STR into this issue etc. and properly close it as a duplicate.

πŸ‡ΊπŸ‡ΈUnited States xjm

Also expanded the CR to be more clear about the actions a site owner should take.

πŸ‡ΊπŸ‡ΈUnited States xjm

Fleshed out the release note a bit to describe the action site owners should take, including the UI label for the permission since it's very different from the machine name.

The fact that this grants access that previously was (incorrectly) denied means it's a minor-only change, so should only be committed to 11.x at this point. One of the first issues for the 11.1.0 release notes, but not the first.

πŸ‡ΊπŸ‡ΈUnited States xjm

Belatedly saving additional credits for folks who helped sort the release note.

πŸ‡ΊπŸ‡ΈUnited States xjm

Okay great, thanks! Let's mark this issue back to fixed, then.

It already has the "Needs followup" tag, which looks like it was left on by accident when @catch filed a different followup in #206. However, it looks like we have two additional potential followups to file, for #230 and the last paragraph in #23. I'll levae it to the respective contributors to file those followups since they will be best able to articulate the issues. Thanks again!

πŸ‡ΊπŸ‡ΈUnited States xjm

I see cases for both casing of the cases πŸ˜‰ which means that following existing external standards is the right call. The only reason we might not do so immediately is if it's inconsistent with other core standards (I don't see a conflict) or disruptive to core.

Here are the places enum is used in core already (outside of the Doctrine fork):

core/lib/Drupal/Core/Database/Transaction/StackItemType.php
core/lib/Drupal/Core/Database/Transaction/ClientConnectionTransactionState.php
core/lib/Drupal/Core/Database/Event/StatementEvent.php
core/lib/Drupal/Core/Form/ToConfig.php
core/lib/Drupal/Core/File/FileExists.php
core/lib/Drupal/Core/Config/Action/Exists.php
core/lib/Drupal/Core/DefaultContent/Existing.php
core/lib/Drupal/Core/Theme/ExtensionType.php
core/modules/system/tests/modules/performance_test/src/Cache/CacheTagOperation.php

I checked and all of them use UpperCamel (following #3443118: Enum cases may need to change to UpperCamelCase β†’ which fixed the config enum).

So +1 from me also.

πŸ‡ΊπŸ‡ΈUnited States xjm

So turns out I was conflating block instances with content blocks, and the block plugin modal that you get when you click the "Place block" link at admin/structure/block with the block library at /admin/content/block. Both places have the "Add content block" button, but they are completely different UIs. My mistake!

It turns out there are issues with the "access block library" permission, but they exist without this patch as well. Manual testing steps.

  1. Install Standard with HEAD.
  2. At /admin/content/block, click "Add content block".
  3. Create a test block.
  4. At /admin/people/create, create a test user that only has the "authenticated user" role.
  5. At /admin/people/permissions/authenticated, grant authenticated users only the "Access the Content blocks overview page" permission.
  6. Log in as the test user.
  7. Go to /admin/content/block.
  8. Click on "Test block 1".
  9. Observe the "Access denied" message, despite the content block being published? In fact, there doesn't even seem to be publishing status for the block.
  10. Log back in as the administrative user.
  11. On admin/structure/block, click the "Place block" button in the Header region, and place an instance of the test block from step 3.
  12. Switch back to the test user.
  13. Go back to /admin/content/block.
  14. Clicking on the test block link still gives "access denied", despite the content block itself being clearly visible in the header.
  15. Switch back to the admin user.
  16. Go back to /admin/content/block and click the link for the test block. The link actually takes you to the edit page, not the view page (there is no standalone "view" page for a content block apparently), which is why it 403s for the test user.
  17. At /admin/people/permissions/authenticated, grant authenticated users only the "Basic block: Edit content block" permission.
  18. Log back in as the test user and go back to /admin/content/block. Clicking on the link for the block now works and takes you to the edit form as expected.

So if you don't have permission to edit a block, the link in the block library should be rendered as plain text instead. But that's out of scope here since it's an existing problem without the patch. In BlockContentAccessControlHandler::checkAccess(), it looks like the intention was for "access block library" to give you permission to view the blocks even if they were unpublished, but there's actually no such thing as an unpublished content block, so that is also weird. You'd have to alter the entity type's base fields for that code to do anything.

Still need that manual testing for the actual case of creating the content blocks (with exact steps to reproduce like I've given above for testing the access content library permission).

πŸ‡ΊπŸ‡ΈUnited States xjm

I closed the superfluous merge request. Unfortunately, the canonical one mentioned in the IS has merge conflicts. So this needs those conflicts resolved. (To receive credit for resolving merge conflicts, the conflict and how it was resolved need to be documented on the issue.) Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

Definitely much better.

Committed to 11.x and 11.0.x. There's a merge conflict on cherry-pick to 10.4.x, so we would need a different backport version.

Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

@acbramley, sorry, I just realized this was actually my fault. I thought I had specifically stated where the comment had come from but apparently it never made it from my brain to the issue comment. :) My bad!

πŸ‡ΊπŸ‡ΈUnited States xjm

While we're discussing it, I haven't don't much testing but I think I echo some concerns from 14 years ago about caching. I haven't tested yet but is the CSRF token going to end up cached in templates using the code in the change log? Should core provide a lazy builder?

This is probably worth opening a followup issue. Discussion on this issue should only be about what needs to go in the release note so that it can be marked back to fixed. @neclimdul, does the current release note snippet in the IS give sufficient info for a site owner to address what you encountered? Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

@larowlan I agree with fixing it so that the "create" permission alone is sufficient to create. What I'm still stuck on is how the block library is useful without the permission. Maybe manual testing steps for using the block library before-and-after would help.

Actually, that made me realize that the change hasn't had any documented manual testing of the main issue with block creation either (oops!) Documenting any STR with manual testing will make it easer for confused reviewers like myself to get to the same page. :)

πŸ‡ΊπŸ‡ΈUnited States xjm

πŸ“Œ Update to jquery UI 1.14.0 beta Downport also needs a decision.

πŸ‡ΊπŸ‡ΈUnited States xjm

As I mentioned on the other issue, I also am not sure if lumping naming with documentation expectations is correct. It should possibly be separate issues.

Production build 0.69.0 2024