Account created on 19 January 2006, over 18 years ago
#

Merge Requests

More

Recent comments

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

๐ŸŒฑ Add an event listing to Umami (to show off core's datetime handling) Active is related. Iโ€™ll try to share more concrete thoughts on this when I can. Definitely important, and I have a lot of experience and opinions about Drupalโ€™s date and event handling. ๐Ÿ˜…

Thanks!
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww
  1. I made a very minor edit to the CR. Agreed itโ€™s worth having.
  2. Good catch on the test comment. Apologies I missed that. Thatโ€™s what I get for quickly reviewing on my phone. ๐Ÿ˜…
  3. Changes look good. Feedback addressed. Pipeline is green.

Back to RTBC.

Thanks,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww
  1. Changes look reasonable to me, and IMHO are all in scope.
  2. Pipeline is green.
  3. I took a stab at a release note snippet (which we'll definitely need).
  4. I'm not sure if this needs a CR for distribution maintainers, too.
  5. Bumping to RTBC so the Product managers will see it and make a final call both on the change, and the need for a CR or not.

Thanks!
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

+1. I have to do this on every site I set up. Admin-only should be the default until folks opt-in to something more permissive.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Iโ€™m okay to keep discussing, but 10.3.0-rc1 is out, so we have very little time to get this done if itโ€™s going to matter at all. ๐Ÿ˜…

To me, it boils down to the costs of leaving it in (hardship for maintainers that like to โ€œchase HEADโ€) vs the costs of removing it (none, other than the time to discuss it). ๐Ÿ˜‰

The primary argument from Berdir against this is basically โ€œchasing HEAD is a fruitless, pointless effort that only leads to more work and sufferingโ€. I pretty much agree with that, and donโ€™t do it myself. ๐Ÿ˜‚ But some maintainers like to do it, anyway, and by one perspective, theyโ€™re trying to do the right thing by staying on top of core development that impacts their code.

As I wrote in the summary, the perspective I do hold is that, when feasible (or in this case, trivial) to make it easier for contrib to put out releases that are as widely compatible as possible, we should.

Not sure who else needs to chime in before a decision is made. I only hope it happens soon. ๐Ÿ™

Thanks,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I sort of mentioned at the parent, but Iโ€™ll say it again here: if this were for discussing actual supply chain attacks we knew to have occurred, weโ€™d need to be doing it privately on the security.drupal.org issue queue. The only reason weโ€™re talking about any of this publicly is precisely because itโ€™s all โ€œhypotheticalโ€ and weโ€™re trying to mitigate vectors proactively before theyโ€™re exploited, not scramble to deal with the fallout of a real attack.

That said, Iโ€™m -1 to this proposal, too. ๐Ÿ˜… Iโ€™d rather make it more clear when owners have changed if a (set of) maintainer(s) has/have gone missing and someone else has taken over than to encourage forking. In my biased opinion, one of the great strengths of Drupal is that we have always enabled and encouraged collaboration over forking as the fundamental model of contribution.

I, too, donโ€™t at all agree with this:

If there is no ability to "adopt" modules the majority of the supply chain attack risk disappears and the ecosystem is protected.

I donโ€™t believe project adoption is a (hypothetical) vector for the โ€œmajorityโ€ of supply chain attacks. Doesnโ€™t mean we should not change anything about the process and the tools related to it, but completely preventing adoption would do much more harm to the ecosystem than it would potentially โ€œprotectโ€.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

It has been an ongoing regret that when I very first implemented the project level permissions and maintainers tab on d.o, that I didnโ€™t build this into it from the beginning. ๐Ÿ˜ข Completely aside from the supply chain aspects and whether end users would care to see it, simply for the benefit of d.o site admins to figure out WTF is going on in various conflicts and disagreements that have come up, it would have been incredibly handy.

Also note that while issues are planned to migrate to GitLab, AFAIK, the long term plan is to keep both project nodes and releases on d.o. So the fact GitLab has a feed for some useful things doesnโ€™t solve that the historical view of changes to the โ€œmaintainershipโ€ is lacking and would be useful, even outside the scope of the parent meta.

Thereโ€™s probably an issue somewhere in the GitLab migration family of issues about how to keep project-level permissions in sync between the two worlds, but I donโ€™t have a handy link. And yeah, the two have different models and not all perms cleanly map back and forth. Itโ€™s an unfortunate situation. If only 17 years ago me knew what I know now. ๐Ÿ˜‚

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

P.s. Iโ€™m all in favor of other meta issues to track other aspects of supply chain security that donโ€™t directly involve the project adoption process. I tend to agree that there are plenty of possible threats that donโ€™t require project adoption. No one should assume this is the only set of vectors worthy of exploration and mitigation. But we need small scope if we ever want to get anything done.๐Ÿ˜…

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

@gisle: please do not take this meta or any of the child issues as a personal attack or condemnation of the work you and others have been doing with the project adoption system. Yโ€™all do a ton of mostly thankless but extremely important work, and Iโ€™m incredibly glad you do.

I believe this was opened as an honest and sincere attempt at โ€œdefense in depthโ€, and mitigating whatever vectors we can dream up before they are exploited and real harm is done.

I donโ€™t believe it is wise to only focus on what has been done (and how) from previous exploits. Malice can come in many forms. People who set their eyes on Drupal will find ways to exploit our community and processes, not necessarily replicate other attacks on other โ€œsupply chainsโ€.

I personally oppose some (many?) of the proposed solutions in child issues. ๐Ÿ˜‚ But Iโ€™m grateful for the discussions, and think there will be some agreeable and actionable changes that come out of it.

Weโ€™re all interested in the same things: preventing exploits and keeping our community and its user base safe. Letโ€™s keep an open mind, brainstorm freely, and assume good faith and goodwill.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

@berdir: Re #212: That's an extremely compelling case for why trying to "chase HEAD" is a headache. Quoting my advice to @jonathan1055 at #3451750-9: Fix: The update to convert "numeric" arguments to "entity_target_id" is deprecated โ†’ :

So my recommendation for right now is to leave your views using numeric, tell GitLab CI to ignore the resulting depreciation notices until youโ€™re ready to ship a release that requires at least 10.3.x, then you can convert your shipped views and reconfigure your pipelines to fail on depreciations (if thatโ€™s what you actually want). ๐Ÿ˜…

However, I'm somewhat torn. You can't conditionally include different .yml in your views config based on versions. There really is no work-around if you want to try to keep up. There's 0 harm in silencing this deprecation in 10.3.x -- folks have years to learn about it and switch. I'm a big fan of making it as easy as possible to be as compatible with as many versions as possible, since ultimately, that's best for end users.

When I pinged @catch in Slack about this (before your comment), he replied:

So leave everything in 10.3 so contrib can actually use the new thing when we get to 10.4?
Silencing or just removing the deprecation in 10.3 seems ok yeah.

So I opened ๐Ÿ“Œ Remove deprecation about Views argument config conversion from numeric to entity_target_id from 10.3.x Closed: duplicate with a green MR so we can make things easier before the 10.3.0 final release.

Let's continue to discuss there, if necessary, and let this giant issue finally go silent for the 90 followers that only wanted their entity reference arguments to work. ๐Ÿ˜‚

Thanks!
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Also, re end of #66, I think this is definitely not just about services.

for example, this is a common parameter for which this documentation would be redundant / pointless:

* @param EntityInterface $entity
*   The Entity to operate on.

Or UserInterface $user, etc.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

We could have a rule that says, more of less:

If any @param statements are required, all must be documented but the descriptions are optional for self-documenting cases.

Eg

/**
 * Does the important and weird thing.
 *
 * @param EntityTypeManager $entity_type_manager
 * @param bool $is_special
 *   TRUE if the special recursively cached access checking 
 *   on even numbered entities is necessary, otherwise FALSE. 
protected function doImportantWeirdness(โ€ฆ): void {
   โ€ฆ
}

Forgive typos / formatting, Iโ€™m typing this comment on my phone while boarding a flight. But hopefully itโ€™s โ€œclearโ€. ๐Ÿค“๐Ÿ˜†

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Re: #10 - indeed, that's definitely how it works with a state_machine field. If we use content_moderation, same story: the โ€œPublishedโ€ flag is still there as a single bit, but the โ€œmoderation stateโ€ is a separate, more granular field with (potentially lots) more options. While configuring states, one of the most significant options (in all such cases) is / would be: โ€œIs this state published/active or unpublished/blocked?โ€ (Not literally that text in the UI, but the spirit of it).

Since this issue is back at the top of my tracker, Iโ€™ll report that the project using state_machine on user entities I was talking about in #8 has launched and this aspect has been working flawlessly. So I highly endorse that for folks looking for an immediate solution to this in contribโ€ฆ

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

That tag was added in #9 as a request to keep the scope limited to class properties. Life moved on. The vast majority of class properties will be auto-documented via constructor property promotion. The whole scope of this issue now is for method params and any non-promoted class members. No follow up needed. Removing tag and restoring status.

Scratch that, moving to RTBC so our committee can review this at our next meeting. I donโ€™t believe anything else is needed for that to be the next step.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

In principle, +1 to being able to validate that people are people. But huge -1 to this specific proposal. It would mean only people with the privilege and resources to travel for in person events could be vetted. That seems like a non-starter to me.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Re #56: see #41. Someone needs to update the summary to use some of those as the examplesโ€ฆ

Totally support this, now that itโ€™s focused on something we can automate.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Mentioned at the core issue, but maybe we want a quick follow up to silence this deprecation in 10.3.x so it only starts getting hit in 10.4.x when 10.2.x is no longer supported?

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I also replied there. But for here, TL;DR: the problem isnโ€™t really about strict schema validation. Itโ€™s that the plug-ins do not exist in 10.2.x. So until folks can drop 10.2.x support, theyโ€™ll have to live with the deprecation warnings in 10.3.x and up that the auto-conversion will be dropped in 12.0.x.

Should we do a quick follow up to silence this deprecation in 10.3+ so folks donโ€™t have to worry about it until theyโ€™re on 11.x?

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

When I get a moment when Iโ€™m not on my phone, Iโ€™ll try to update the CR for some additional clarity. But a few points here now that Iโ€™ve slept:

  1. The change to fix the bug was to add new argument handler plugins that properly deal with entity references instead of using entity IDs as numbers. These made it into 10.3.x and up, but not 10.2.x and down.
  2. If you ship a view still using numeric itโ€™ll keep โ€œworkingโ€ as well as it always has.
  3. You canโ€™t change your default view to use the new plugin, strict config schema or otherwise, in 10.2.x since the new handlers do not exist there.
  4. But what you can (and probably should) do is to ignore the deprecation warning in 10.3.x about it until you can safely drop support for 10.2.x and require at least 10.3.x.
  5. This deprecation is about the fact that the automatic conversion to the new/shiny handlers will be removed in 12.0.x. So youโ€™ve got plenty of time and things will be auto-converted for you.

So my recommendation for right now is to leave your views using numeric, tell GitLab CI to ignore the resulting depreciation notices until youโ€™re ready to ship a release that requires at least 10.3.x, then you can convert your shipped views and reconfigure your pipelines to fail on depreciations (if thatโ€™s what you actually want). ๐Ÿ˜…

Thanks / apologies,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Oh, I misread. Yeah, if youโ€™re running against 10.3.x, you need to update any default views to use the new handler. Maybe we need another CR to clarify?

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Quite confused. Nothing was backported to 10.2.X. I donโ€™t see how this could possibly be causing any test failures. But itโ€™s late and Iโ€™m not of my right mind, so Iโ€™ll try to look again when Iโ€™m sober and have had some sleep. ๐Ÿ˜‚

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

My comment is linked in the summary, but copying here for visibility:

However, I had to completely rewrite ViewsConfigUpdater::processEntityArgumentUpdate() to operate on entire View entities, instead of using all the per-handler plumbing we've got. To my great dismay, if you copy the existing patterns, ViewsConfigUpdater process functions make a bunch of changes to handler config, then try to save the view, but that instantiates another ViewsConfigUpdater object (this time, with deprecations enabled) to do all the work again. ๐Ÿคฏ So apparently, The Right Wayโ„ข๏ธ to update a view with all this plumbing is that your process function has to directly update the $view (which isn't even normally passed in to your process function if you think you're just dealing with handlers), not just fix the $handler. ๐Ÿคฆโ€โ™‚๏ธ

It's probably out of scope to actually fix that as part of an issue with the title "Document ...", but maybe we want to expand the scope here?

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Ahhh, gotcha. Thanks for explaining.

ed1e913d2bb2fb6c3a469a09a26d1ae575746e99 looks fine -- not even a "typo" really, only adding a , to the warning message to improve legibility.

Definitely still RTBC. Let's get this in.

Thanks again!
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Added Unit test coverage that shows the bug and proves the fix.

We probably want to extend some Functional coverage for this, too, since there might be other spots that blow up in this case, and it'd be nice to have an end-to-end test to verify it.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Here's a quick start. Needs tests, but wanted to get an MR open ASAP.

Elevating to critical since this is potentially very bad. ๐Ÿ˜ฌ While there are guardrails for the version strings themselves when creating releases on d.o, there's nothing to prevent people from having typos in the core_version_requirement they put into tagged releases. Update manager needs to be resilient to those sorts of mistakes, not hard crash with uncaught exceptions. Especially since we generally prevent anyone from fixing a broken release, so once there's a problem, we've got it "forever".

Fixing the summary, both formatting, and some technical details.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

p.s. Doesn't really matter, but I'm restoring this to a bug. ๐Ÿ˜…

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

FYI: Both ๐Ÿ› Fix label token replacement for views entity reference arguments RTBC and ๐Ÿ› [PP-1] Use labels in Views argument summaries for entity references Active are committed and will be available in 10.3.0 and up. ๐ŸŽ‰ At last, these bugs are fixed! We copied over credit for the effort here to #3442227, too.

Enjoy,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Oh, the irony! ๐Ÿคฃ

https://git.drupalcode.org/issue/drupal-3422537/-/jobs/1747113

Issues found:
./.gitlab-ci.yml:404:202 - Unknown word (unshallow)
CSpell: Files checked: 1, Issues found: 1 in 1 file.

Anyway https://git.drupalcode.org/project/drupal/-/merge_requests/8258/diffs looks excellent. That's more or less exactly what I had in mind.

I'm not totally following the results in https://git.drupalcode.org/project/drupal/-/merge_requests/8259/pipelines to see that this definitely fixes the problem. But on first principles, the merge-able MR (!8258) looks right, and based on the history of these failures and what we understand about their cause, it should solve it.

Super minor cost to run git diff twice, and that avoids always doing a full clone (which would be much more costly).

Ship it! ๐Ÿ˜‰

Thanks,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

p.s. Added 10.3.x versions to the CR and published it:
https://www.drupal.org/node/3441945 โ†’

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Woo hoo! ๐ŸŽ‰ Thanks, @catch!!

I got the follow-up ready if anyone following here is interested in reviewing (and hopefully RTBC'ing) that one:

๐Ÿ› [PP-1] Use labels in Views argument summaries for entity references Active

Thanks again! Very happy to see this fixed.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

So long as the threads are addressed and the tests are actually testing this, I'm happy. So signing off as subsystem maintainer.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

@smustgrave (bless his soul!) traded me a review here for his reviews of ๐Ÿ› Fix label token replacement for views entity reference arguments RTBC (which just landed for 10.3.0 ๐ŸŽ‰). Finally circling back to uphold my end of the bargain. ๐Ÿ˜‚

In principle, +1 to the general approach of marking the recommended update as "Recommended security update" if it's a security update from the same branch as you're currently running. We want folks to get off known-insecure versions ASAP, and a security release from the same branch is the least disruptive thing to do, so that's what we should "Recommend".

However, I left a detailed review on the MR and opened 9 threads, some of them fairly easy suggestions to review, but a couple of important ones.

I've only looked at the code, haven't tried any manual testing.

One of my key review points is that I don't think the test changes are testing anything about the new functionality, so we need to fix that before we can have confidence from the GitLab pipeline. Probably would be good to do some manual testing, too.

Thanks for keeping this alive and moving forward, and apologies for the long lapses in my availability to review this...
-Derek

p.s. Crediting @smustgrave for all the efforts here, both reviewing and tracking subsystem maintainers, and myself for the detailed review.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww
  1. Rebased so the issue fork branch is clean and back to only the test and the fix.
  2. Updated the MR description now that that's done.
  3. Updated the summary here with a more common use case (nodes referencing taxonomy terms) and updated remaining tasks.
  4. Main pipeline is all green.
  5. Test-only failed exactly as intended:
There was 1 failure:
1) Drupal\Tests\views\Kernel\Handler\ArgumentSummaryTest::testArgumentSummary
Failed asserting that '1 (4) 2 (2)' [ASCII](length: 11) contains "rejeaplf (4)" [ASCII](length: 12).
/builds/issue/drupal-3442227/core/modules/views/tests/src/Kernel/Handler/ArgumentSummaryTest.php:146
FAILURES!
Tests: 1, Assertions: 14, Failures: 1.

Ready for review (and hopefully quick RTBC). Would be lovely to also get this in before 10.3.0.

Thanks!
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Pretty sure this is duplicate with ๐Ÿ› Fix label token replacement for views entity reference arguments RTBC which just landed and will be included in 10.3.0

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Woo hoo, the blocker is in. This needs a rebase, but then should be ready. Iโ€™ll do it later tonight when Iโ€™m back at a computer, unless someone else is inspired to do it before then.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

- Shortened the keys
- Added the missing config schema

Y'all can write tests if you need them. I'm way out of time on the project where I need this functionality, and it's working fine for me.

Cheers,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Implemented the separate 'Matching strategy' radios and corresponding logic changes in evaluate(). Also replaced the deprecated user_role_names() call with the recommended alternative. Light local testing is working well. Probably wants automated tests, but I don't have time for that now (about to board a flight).

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Addressed most of #6, but not the fancy roles operations stuff at the end. Converted to MR, hiding patches.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

dww โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

The core Update Manager cannot correctly update modules like address that have external dependencies. You need to use composer to manage your updates if you use this module.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

@Jaypan: Great, thanks!

@sickness29: Welcome aboard!

Cheers,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Hi! Thanks for the offer. Apologies for the delays. Iโ€™ve been incredibly slammed in many areas of my life for the last many months.

Given my experience of your help and good work on datetime_extras, I would gladly grant you access. Alas, I donโ€™t have permissions to do so myself. Only Jaypan has that power at the moment.

Assigning to them and bumping status.

If nothing else happens in a few weeks, maybe we can convert this into a request to make both of us full maintainersโ€ฆ

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

p.s. I tried triggering the test-only GitLab CI job, but it's all confused by the #2640994 changes. Probably test-only via GitLab won't tell us much until that issue lands. However, locally, if I revert the fix and just run the new test, I see:

There was 1 failure:

1) Drupal\Tests\views\Kernel\Handler\ArgumentSummaryTest::testArgumentSummary
Failed asserting that '1 (4) 2 (2)' contains "phk5vh2z (4)".

Which is exactly what we'd expect. The summary is only showing term IDs and the node counts, not the term labels...

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I searched (a lot), and didn't find any test coverage of argument summaries. So I wrote a Kernel test for this on a flight earlier today. The MR still has a ton of changes since this needs ๐Ÿ› Fix label token replacement for views entity reference arguments RTBC to land. But I squashed that into a single commit in this issue fork, so it'll be easy to rebase that out of here once that issue lands. If anyone wants to review, the actual changes are just 3 commits:

https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...
https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...
https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Wow, what a colossal PITA. ๐Ÿ˜† Guess it's good I jumped through all those hoops, since:

  1. There were indeed some test views with stale argument plugin definitions that needed to be manually fixed, including the view for testing this new functionality. ๐Ÿ˜‚
  2. EntityArgumentUpdateTest was making some slightly bogus assertions.

However, I had to completely rewrite ViewsConfigUpdater::processEntityArgumentUpdate() to operate on entire View entities, instead of using all the per-handler plumbing we've got. To my great dismay, if you copy the existing patterns, ViewsConfigUpdater process functions make a bunch of changes to handler config, then try to save the view, but that instantiates another ViewsConfigUpdater object (this time, with deprecations enabled) to do all the work again. ๐Ÿคฏ So apparently, The Right Wayโ„ข๏ธ to update a view with all this plumbing is that your process function has to directly update the $view (which isn't even normally passed in to your process function if you think you're just dealing with handlers), not just fix the $handler. ๐Ÿคฆโ€โ™‚๏ธ

I guess I'm now in the exclusive club of ~5 people on Earth who understand how this class works. ๐Ÿ˜ฌ ๐Ÿ˜‚

ANYWAY, we're back to green pipelines on 11.x and 10.3.x. I've given up on the 10.2.x MR at this point. It works great for the project where I needed this fixed, all the additional hassles are irrelevant to that site, and this is clearly not getting backported. I just hope this can still land in 10.3.x!

Thanks,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I got a bit more clarity from @catch on what's expected here. I pushed some commits to the 11.x and 10.3.x MRs to trigger deprecations when ViewsConfigUpdater::processEntityArgumentUpdate() changes a view outside of post_update. I'm not sure on the wording of the deprecation message, that could use another pair of eyes. Also unclear if we "need" tests that this deprecation plumbing works as expected. I sure hope that's not my responsibility here. ๐Ÿ˜…

I'll keep an eye on the pipelines to make sure I didn't break anything, but I hope this is now actually RTBC.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

You're probably hitting this core bug: ๐Ÿ› Fix label token replacement for views entity reference arguments RTBC . Sadly, this project can't help you with that. It's a bug in the Argument plugin itself, and that can't be solved by an ArgumentValidator plugin like the one provided here.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I asked in #bugsmash for help on this. @catch pointed me to ๐Ÿ“Œ ckeditor5 and editor module tests rely on hook_editor_presave() bc layers Active , but that issue has nothing to do with ViewsConfigUpdater. I still don't exactly understand what's being asked of me.

Views is doing this:

function views_view_presave(ViewEntityInterface $view) {
  /** @var \Drupal\views\ViewsConfigUpdater $config_updater */
  $config_updater = \Drupal::classResolver(ViewsConfigUpdater::class);
  $config_updater->updateAll($view);
}

@alexpott, are you proposing we should change the API for ViewsConfigUpdater::updateAll() to take a 2nd argument to determine if we're being called from presave or post_update? Or do you propose that views_view_presave() checks the return value from ViewsConfigUpdater::updateAll() and if it ever gets back TRUE it should always trigger a deprecation?

Can we pretty please move all such complications to a dedicated follow-up, and fix this before the 10.3.0-beta1 ships? I still fail to see why this poor bug needs to be the one to deal with all that additional functionality.

Tentatively moving back to RTBC to get more committer eyes on this.

Thanks,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Probably itโ€™ll be 11 before this happens, but at least bumping to 10 for now

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Looks good to me, though we probably want @phenaproxima to formally accept in here before this is committed.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

In very mild local testing, the change in the MR seems to work. This is the first time I'm looking closely at payment gateways at all, much less the gory details of Stripe. Would love some reviews / pointers on if this is legit. ๐Ÿ˜‚

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Rebased the 11.x MR to resolve conflicts.

@alexpott: Not sure what you mean with this:

I think we need to trigger deprecations from the ViewConfigUpdater when it is fixing a view during preSave that's not done as part of the post update. This way modules will know that they have to update their shipped content.

Sounds like scope creep to me. I don't want to be rewriting ViewConfigUpdater to add new functionality like this to try to fix this bug. Can we address that in a follow-up?

Thanks,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I opened an MR thread to document how the diff is removing an exception. I'm a little sad to change the title at all, but how's this?

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Hate to ask, but do we need a change record for this?

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I landed here chasing various things. For how long this issue has been open, how many comments, patches and effort went into this, I was amazed / shocked / delighted / heartbroken that the diffstat on the changes tab in the MR is: +7 / - 3. ๐Ÿ˜‚ Thrilled to see it's actually RTBC at this point, and hopefully going to land soon. +1 to the RTBC. The code changes are great. The test change is that we now get to check for better outcomes to a common DX problem. ๐Ÿš€ it!

Thanks,
-Derek

p.s. Starting to save credit. Could use a closer look before commit.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I tried this locally:

-  public function summaryName($data) {
+  public function summaryName(array $data): string|\Stringable|NULL {

Then this:

./vendor/bin/phpunit --debug -c core/phpunit.xml core/modules/views/tests/src/Functional/Plugin/EntityArgumentTest.php

Which resulted in this:

There was 1 failure:

1) Drupal\Tests\views\Functional\Plugin\EntityArgumentTest::testArgumentTitle
Test was run in child process and ended unexpectedly

Reverting the type changes to summaryName() and that test is passing fine. So either I botched it somehow, or this just won't work until we add types to Views plugins (or at least to Argument plugins). /shrug

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Thanks for taking a look!

Re types: I donโ€™t believe so, since this is implementing a parent method that doesnโ€™t have types and thatโ€™d be an api change. I believe. ๐Ÿ˜‚ Iโ€™m honestly not sure what our policy is on that.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Looking at the rest of core/modules/field_ui/tests/src/Functional/ManageDisplayTest.php makes me cringe. Wow that's a lot of separate (tiny) test methods. ๐Ÿ˜ข Obviously out of scope to refactor all of this, but probably worth a follow-up.

That said, I'm not sure what I'd suggest for this issue, so maybe adding a new test method is fine. Restoring NR, but it smells a little fishy to me. ๐Ÿ˜…

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Love this. Added a note about test efficiency to the MR. I don't think we need a whole new test (and therefore, complete install of Drupal) for 2 assertions. ๐Ÿ˜… Let's move these into an existing test to save time / CO2 / DA $.

Otherwise, seems like a win to me, and probably very close to RTBC.

Thanks!
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Thanks for the reviews and RTBC! I pushed the bool return type to ViewsConfigUpdater::needsEntityArgumentUpdate() to both 10.3.x and 10.2.x MRs. Thanks for spotting that.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Do we need tests for this? I put that as the remaining task in the summary.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Adding STR. I tried testing locally and at first failed since Claro doesn't have the bug. You have to turn off Claro to see it in action.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I know this is tagged novice, which I am not ๐Ÿ˜‚, but @catch pinged in #bugsmash about this since it's time sensitive. So I pushed fixes (I think) and opened an MR. My local can't run 11.x right now, but I'll see if this applies to 10.3.x and try to test it there.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

dww โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Note to reviewers: Until #2640994 lands, the "Changes" tab in the MR is a ton of stuff you don't need to look at. This is the only change being proposed here:

https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...

Thanks,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

This is technically still blocked on ๐Ÿ› Fix label token replacement for views entity reference arguments RTBC . I pushed a single commit with all the changes from that issue into this issue fork, then another commit to actually fix the bug here. It'll be trivial to rebase this and remove the 1st commit once #2640994 lands. But I wanted to get this moving in the hopes we can fix both in time for 10.3.0.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

The 11.x branch needed a rebase (with some fairly trivial conflicts) due to ๐Ÿ“Œ Removed deprecated code in taxonomy module Fixed

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Off topic from this issue, but you should never point composer at a GitLab MR. Anyone could push broken or malicious code to the MR but the path doesnโ€™t change and composer will happily try to apply the now broken or vulnerable code to your site.

Download the patch. Store it locally. Tell composer to apply that. Iโ€™ve got a clean checkout for core development. Apply it there. Make a new patch. Put that in your composer.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Re #5: Thanks for putting #3 into the summary.

Where does 'media' come from in "task(media):" ?

The spec:

<type>[optional scope]: <description>

My interpretation from #3:

The optional scope would map to an issue's Component. That's a nice touch, but I'm not as set on including that as the other aspects.

Re: #6: Maybe it's only because I've been doing this way in Drupal and every other kind of thing I code for decades, but having the "issue" ID prominent in the summaries is essential. There are lots of places in Git (CLI or otherwise) where *all* you see is the 1-line summary. I'd hate to not know the unique ID for the issue when scanning those.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Love it. I do stuff like this a lot in tests. I basically can't stand to see data providers in Functional tests, and heaven-forbid in FunctionalJavaScript.

The results in #8 are mind blowing, given the small size of the diff. 8:11 to 0:24 is more than a 20X speedup. ๐Ÿ˜‚

Ship it!

Thanks,
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I'm new to these tests. Took me a moment to understand why we're moving assertLogError() outside the check for if we've written anything to the logs. But then with grep and looking at more context outside of the MR diff, I see that that function relies on $this->expectedLoggedErrors, which is only set in the two functions where assertLogError() is now found.

At the risk of slowing down this important fix, I have to ask: should that method really be something like:

assertNumberOfLoggedErrors(int $expected): void

Why bother with the expectedLoggedErrors property at all? It's only set exactly twice with hard-coded ints each time:

./core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php:    $this->expectedLoggedErrors = 27;
./core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php:    $this->expectedLoggedErrors = 39;

We never increment or anything. It's basically just a constant. Why not pass it directly where we need it?

That said, I'm going to RTBC this, since my refactoring concern doesn't need to hold this up. If y'all agree the above would be cleaner, but don't want to do it here, it'd be a novice follow-up.

Thanks!
-Derek

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Pipelines are all green again, except the functional JS random fail in the 10.2.x MR mentioned above. I doubt the RMs are actually going to backport this to 10.2.x, and that MR is mostly for folks to patch their own live sites with until 10.3.0 is out and they can upgrade to it...

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

As usual, this fell off my radar. Tagging to be smashed. I can't do a full subsys maintainer review anytime soon, but trying to remind myself to get back to this ASAP.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Ahh, thanks. The 11.x MR was failing due to ๐Ÿ“Œ Remove the 9.4 database dump fixtures from 11.x Active . ๐Ÿ˜… Pushed a commit to use the 10.3.0 fixture instead of 9.4.0. That works locally in the 10.3.x MR. My local can't run 11.x ATM. ๐Ÿ˜‚ Also rebased both MRs to latest 11.x and 10.3.x core. We should be all green again. ๐Ÿคž

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Not sure what you mean. The ones I opened are very clean. ๐Ÿ˜‚ I donโ€™t have edit access to 7091, so the title and description are lacking, but does that matter? The summary explains what all 3 are for.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I needed this, and it was quite simple, so I opened an MR for it. There's no .gitlab-ci.yml for this project, so there's no pipeline, but it works, I promise. ๐Ÿ˜‚

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

dww โ†’ made their first commit to this issueโ€™s fork.

Production build 0.69.0 2024