For #16, it's actually usually preferred to document the output of the failing test for the test-only in the job in a code tag, like so:
// Failed job output here
N methods failed Y asserts completed in blah blah
It's not sufficient that it fails; it needs to fail in the right way and then we document that. 🙂 Thanks!
Counterpoint: I think there is a discoverability issue here with removing it (or any field module) from core until Package Manager is fully stable and in Standard to do its thing. So I would suggest keeping it in core for an additional major.
I agree, that would probably require a lot of coding, and probably could never really function as expected anyway ...
It's not just that it would require a lot of coding. This is an official Security Team and release management statement that we already made an official decision, back in like 2014, that it was too dangerous from a security perspective to clean up the view the way we clean up other third-party config stuff. I am just documenting that past decision again for the record of this issue. 🙂
I don't necessarily agree that disabling the View is a non-option; we should discuss the pros and cons with design/usability and with the subsystem maintainers for Views and config.
I think this should be lower frequency (say once per minor to start), but published on https://www.drupal.org/about/core/blog → .
The initial post should explain what the initiative is and be an introduction of sorts. The next one could then give a progress update and maybe add additional conceptual content. I suggest one when we finish drafting it and a second one before the December minor release window.
Committed the backport to 10.6.x. Thanks!
NW because I think this will break translations. The comma-separated list was a compromise to inline the "proper" way of doing this. Thanks!
👍 Restoring the RTBC for signoffs.
BTW I debated bug vs. task (though it remains critical either way). I think enough people experience this as a very bad surprise that it should still be treated as a bug, even though there is technically a warning message and it is technically working as designed.
A better design in general might be to require a manual step beforehand to remove the config, similar to the content removal operation. That way the user has to think more about what they are doing. However, for Views, we have the option to disable the view. It's broken without its dependencies, but the dependencies could be restored from an export or from reinstalling a module etc.
Yes, this is a duplicate.
I came to this issue thinking "how on earth did that possibly regress" but the title is wrong per #46 / #47. Updating the title to what the actual goal is.
I agree that deleting a whole view is potentially bad even with a warning, but we also don't want to make it impossible to uninstall modules. Some sites have hundreds of views and so manually editing every one is not feasible.
We did spend a long time before 8.0.0 deciding on exactly what we wanted this behavior to be for config dependencies in general, and the current behavior in HEAD is what we agreed on. We can't try to repair the view automatically because we don't know how the module's plugins or whatever are used and access bypasses etc. could be introduced.
The DA has deployed a possiible fix for this, but it will take several hours for the content to be rebuilt.
lol. I had forgotten how salty they get about that. We can say "Linux or similar".
Postponing pending the move to contrib. I had not realized that 📌 [meta] Tasks to deprecate the Contact module Active was only blocked on trying to get access to the Contact namespace for months (which we don't need to do for core module namespaces where the module is being moved to back to contrib).
In that case maybe we should backport this for 11.2/10.5?
I granted Andy and Lee full maintainer privileges. Fill yer boots!
For what it's worth, in cases like this where someone is squatting an ancient core module namespace from before it was added to core in the first place, we are not actually required to follow the full maintainership offer process. We can just appoint core designees to the module.
Also it's Dave Reid for goodness' sake. He is not going to mind. 🙂
Committed to 11.x, and backported to 11.2.x for parity since it changes only the test suite. Thanks!
Clarifying title, since my kneejerk reaction was "wait that's backwards". 😀
Thanks for working on this. I recommend some additional documentation on the format of the paramenter. We don't need to repeat the full $to docs, but we should at least say it has the same format as that parameter. Thanks!
@java008 We don't generally handle support requests in the Drupal.org core issue queue, so you will want to look elsewhere (like the Drupal community Slack) to get support for your issue. Thanks!
Thanks Juampy!
Committed to 11.x, 11.2.x, 10.6.x, and 10.5.x. I also updated the core project node and the core maintainer channel.
Discussed with @catch. We are okay with no polyfill. However, that would mean officially dropping support for running the core test suite locally on PHPUnit 10 (currently impossible anyway due to the sebastian/diff constraint, but ideally that will be fixed before the Aug. patch release window). So, we will need a CR and a release note.
Oops, I missed a tag (for testing topic maintainer review).
What docs are these?
Um... well... that's a good question.
- One of them is an unpublished Google doc about release manager process for minors.
- Maybe also in the definition of maintenance minors → although that is a bit high-level so maybe not.
- And something on the continuous upgrade path policy → actually since this is about parity between the old and new major branches, although I keep struggling with exactly where to make any changes to that policy in its content.
Again, for point 1 in the IS, I think it should go under patch releases rather than minor releases. Or that's what we're proposing for review, anyway. Non-disruptive testing API additions are already allowed in normal minors as they fall under the category of "new APIs or API improvements with backward compatibility".
It might be worth just special-casing @quietone as an interim mitigation because this is a barrier for core release process every single minor release.
Post update hooks are not oop.
I meant things triggered by them. If they themselves were being ordered by this mechanism I would've definitely marked it critical. (I checked that first.) But yeah, let's leave aside what exactly happens when or whether this is possible. The base issue (and the case for critical) is that there are so many different ways this could break advanced usecases that it's hard to rule out the possibility of the "definitely critical" categories of issue (security, data integrity, upgrade path). So let's err on the side of the higher priority which we do when it's borderline in general.
@godotislate, that's a great point!
@nicxvan pointed out that we should also add test coverage that the hook order is correct with or without the missing hook.
I talked myself into critical. This mostly only affects custom code or rather advanced contrib modules that integrate with other contrib modules, which would usually limit the scope to major, but if someone hits this on any one of the many hooks that act on CRUD operations it could affect data integrity or even something triggered in a post-update hook (and therefore the upgrade path).
Can we add some explicit assertions about the existence of the non-existent hook fixture definition in the registry or whatnot? Otherwise the file could be removed without tests failing. Thanks!
This is at least major. A case could be made for critical.
Apache on Windows is technically a thing, right? So this technically should have all the signoffs?
Re: the proposed text. I think we should say:
It is recommended to use Linux for hosting Drupal websites. Running Drupal directly on Windows in a production environment is not supported.
...because we don't want to confuse the uninitiated if they see the sentence out of context and make them think they can't use Windows or Darwin or whatever for local dev (or worse, accessing sites, lol).
Thanks @benjy for all your contributions!
Committed to 11.x and cherry-picked to 11.2.x, 10.6.x, and 10.5.x. I also removed permissions from the core project node and the maintainers channel.
All the core leadership team members either endorsed this policy proposal or had no opinion on it. So the next step is to create a governance followup issue. NW for that I guess?
Technically the PHPUnit 10 support question is a release manager review thing, so getting a second opinion.
Correcting my comment from #12: The core constraint is still ^10 || ^11; it was dropping support for sebastian/diff 5 that accidentally made PHPUnit 11 the de facto core requirement. But same conclusion. 🙂
Committed to 11.x and cherry picked to 11.2.x, 10.6.x, and 10.5.x. I also added @fathershawn with subsytem-maintainer privileges on https://www.drupal.org/node/3060/maintainers → and invited him to the private Slack maintainer channel.
Welcome to the team!
Also to our minor spreadsheets.
And updated credits.
Oh, belatedly confirming #63 -- @alexpott and I already discussed that in Slack last week but it looks like our consensus that the followups are covered did not make it back onto the issue. Thanks!
Untagging from #70; thanks Ken!
Ah good call; I did not actually review it. Thanks @prudloff.
Improve instructions about the maintainer-only clone and also clarify that other clones should use the read-only remote for the main project.
Also a potential highlights thing, especially if we end up with other site builder UX tweaks.
Saving reviewer credit for @longwave. Also looks like I had missed @smustgrave's changes and nontrivial rebase back in May so saving credit for that too.
#239 explained the diff weirdnesses with Umami config that were making me scratch my head, thanks!
A call in the HtmlTag element itself; very nice!
FWIW regarding #22 which I did not read closely before, I think that is works as designed.
The meanings of these two actions are much clearer with the updated form texts, thanks! Posted a couple small suggestions.
#13 sounds good. Also fine with task although maybe the issue should be postponed for contrib-land then.
Re:
Leaving in NR since this is tagged for subsystem maintainer review which is not me.
If it's blocked on subsystem maintainers for an extended period in general it can also be escalated to framework managers, although in this case since a subsytem maintainer is also a release manager someone RTBCing it with the tag on is fine as well. Meanwhile though I'm also going to ping the other active maintainer for the subsystem since I still don't have time to look at this this week.
Committed to 11.x, and backported to 11.2.x since it's fixing a regression in the demo profile, which has a relaxed BC policy anyway. Thanks everyone!
Adding credits for maintainer signoffs, and for @smustgrave for manual testing.
Adding reference issue.
Adding credit for @quietone.
Maybe we should add this to docs in addition to the metas?
This does make me a bit nervous, but backporting empty update hooks to patch releases would be not-nice, so I guess we're sort of stuck for 10.5.x.
Since we've officially deferred disruptive deprecations, 10.6.x is more likely to be API-compatible with 11.2.x than 10.5.x was with 11.1.x, so I agree that release notes are probably sufficient for the next minor. However, I do think that we should resume adding them again with 12.1 and do so consistently in the future. We could even script this, honestly.
Is the text proposal just this?
Testing API additions should be backported
That's not grammatically parallel with the existing sections. We would want to add a bullet with just the nominal phrase ("testing API additions").
Also, they're already allowed in minor releases. The idea would be to allow them in not only maintenance minors, but possibly also the patch branch also. So we would add it to that third section.
We might want to qualify it somehow, e.g.:
- non-disruptive testing API additions
Regarding #4, maybe that's an additional bullet for maintenance minors:
- other easily backported test improvements
(...or something; there might be a better version of that.)
Yes, please compare the two merge requests, ensure one includes the full change set, and close the other. Thanks!
One of the issues with the comments is grammar. They're not grammatically correct English.
I think it is relevant to document which langcode behavior we expect to use as that is the biggest gotcha of the whole API (and also exactly what is under test).
That said, the fact that this comment is repeated N times also hints at possible refactorings. (Out of scope, but might make the test easier to follow and reduce the need for boilerplate docs since helper methods or data providers would be more self-documenting.)
Aside: This issue is what happens when we descope the docs gate from something!!
I don't think PMNMI is the correct status? We just need proposals for better comments. (Another node-access-not-actually-node issue.) It's tagged "Needs IS update", which corresponds to "Active" when there's no MR (or NW when there is one). Thanks!
I spoke to @ckrina a bit about this in Slack and I think we should postpone this issue pending another governance issue. We might end up with no active individuals in this role currently, but we might still want to add it back to the governance. Usability topic maintainers would provide support in a single subset of the governance domains covered by the much higher-level strategy owned by the UX managers, similar to how a subsystem maintainer provides architectural decision-making for a single subsystem while a framework manager provides that decision-making when multiple subsystems are affected. Governance followup hopefully forthcoming.
.
Forgot to post my diff for #8:
https://www.drupal.org/node/3530875/revisions/view/14061550/14061628 →
Saving additional credits.
@mondrake Right but that will only affect 11.3.x and HEAD. This is specifically about the text of the 11.2 announcement. I think the sebastian/diff issue being in the known issues covers the rest.
Whoops, so PHPUnit 10 is still allowed, just not the default. Updated again:
https://www.drupal.org/node/3530875/revisions/view/14061515/14061550 →
Good catch! This is probably due to the fact that the change dropping PHP 10 support landed late. (Note that we've also tentatively discussed re-widening the constraint.)
Fixed like so:
https://www.drupal.org/node/3530875/revisions/view/14038203/14061505 →
Saving some additional credits for reviewers and maintainer triage.
Locking issue to prevent further comments and patches from being posted. This was fixed in ✨ Continuation Add Views EntityReference filter to be available for all entity reference fields Active and is available in all supported versions of Drupal core.
Locking issue to prevent further comments and patches from being posted. This was fixed in ✨ Continuation Add Views EntityReference filter to be available for all entity reference fields Active and is available in all supported versions of Drupal core.
I confirmed with @phenaproxima that in HEAD, the binaries are removed entirely, so the change proposed by this issue is adding an internal configuration to make the composer binaries retained and read-only at the given path.
Composer is only a dev dependency for core, not a production one, so this mainly becomes important for sites that have Package Manager enabled (i.e. Composer is a production dependency). If this passes security review, then it's essentially a feature addition of a single internal constant and the binaries.
However, other sites could be affected depending on the sophistication of their deployment workflows. Ideally sites' production builds will be deployed with --no-dev, but we can't be sure this is the case everywhere, so it's a bit dangerous to suddenly have these binaries popping up everywhere in unpredictable situations.
I asked @phenaproxima to consider instead making this an optional configuration that is off by default, but that Drupal CMS (and perhaps later Package Manager itself) enables. That way, it could be backported safely with very little API surface or risk to existing sites, but Drupal CMS could also get access to the configuration immediately. This would also be a security hardening: sites or applications would need to opt into making the binaries available, and they would remain unavailable in the default case for deployments where they are not needed.
This will also need a CR and a release note.
Thanks!
I agree that it makes sense for the contact form to be 404 if the email address is missing.
However, I disagree that providing no warning or user to the administrator of this fact is a good choice. It's right there on the same form; we can provide some sort of warning about it.
Not sure what the bot is on about but this is like a two-character diff in a file that's almost never updated; it will apply easily even if the source branch is out of date. Restoring RTBC.