Account created on 1 February 2012, over 13 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States dcam

I've been following this issue for years longer than I've been the Link module maintainer. I've also wanted to set a default text value for links while letting the URLs be customized per node. Now that I think about it, I still manage that application where I wanted that ability.

That said, I understand the potential usability issues at play here. I started thinking about other possible solutions and came to a realization that ironically was suggested back in 2016 in comment #43 by @Ilesyt:

I would like an option where could I input a link text in the 'Manage display' when editing the field display.

The need that this issue is looking to solve would be really easy implement with a field formatter that has configurable default text. And wouldn't you know it, there's a module that does it: Fixed text link formatter . So if you need default link text functionality, then I advise looking into it.

🇺🇸United States dcam

Postponing for the reasons given in #4.

🇺🇸United States dcam
🇺🇸United States dcam

I see from the comments that naming the property full_url wasn't necessarily a popular choice. I'm not fond of it either. It isn't descriptive or meaningful.

A resolvable, or dereferenceable, URI is a URI that actually points to a functioning website.

I renamed the property from full_url to resolvable_uri. I wouldn't mind resolvable_url, but I decided to stay with "uri" to keep the association with the original uri property. Feel free to provide feedback or other suggestions.

I recreated the MR for Drupal 11. The original MR was hidden and a new one was created because usually changing the MR version breaks everything. Recreation is often the easiest way forward. I had to fix an issue with the JSON:API ShortcutTest. There's a comment on the MR about it.

The issue summary and change record have been updated.

🇺🇸United States dcam

I don't think there would be BC issues since this is autowired and probably no one is extending this, but switched it over anyway.

Me either. But who wants to deal with that one site that has it overridden and shows up to find out who broke their stuff?

Feedback has been addressed. Happy to RTBC this now.

🇺🇸United States dcam

dcam changed the visibility of the branch 3066751-link-fields-in to hidden.

🇺🇸United States dcam

dcam changed the visibility of the branch 11.x to hidden.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

I am not fond of the changes in this patch. It would need a lot of work before it could be accepted.

Support for attributes is intentionally kept minimal and left to contrib to extend. And as it happens there's a contrib module that does almost exactly what this patch is proposing: Link Class . As a result, I am going to close this issue as a "won't fix."

On a related note, as the module's maintainer I am more interested in having a plugin system for attribute support than continuing to add one-off implementations. See Add features of link attributes module to core's link module Active .

Credit has been granted to those who worked on a patch.

🇺🇸United States dcam

I just realized that the issue summary doesn't mention validating email: hrefs at all. So everything related to that is probably out-of-scope.

🇺🇸United States dcam

It's currently failing Drupal\Tests\Component\DrupalComponentTest::testNoCoreInComponent() with the error:

Checking for illegal reference to 'Drupal\Core' namespace in /var/www/html/core/lib/Drupal/Component/Utility/Xss.php

I assume it's because we use Drupal\Core\Template\Attribute in the moved function.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

The MR needed a rebase badly. I took care of it.

I also updated the issue summary. There was no mention of the changes to the LinkWidget settings. I'm not 100% sure they belong in this same issue, but since this was already RTBC twice I won't stop it.

Since this hasn't undergone a usability review I'm setting the status to Needs Review.

🇺🇸United States dcam
🇺🇸United States dcam
🇺🇸United States dcam

I'm postponing this on Allow to select reference method in link field type for internal links RTBC . There are a number of identical changes between the two issues. But the other one has been RTBC twice already whereas this one needs more work.

🇺🇸United States dcam

I'm closing this in favor of Allow to select reference method in link field type for internal links RTBC . Because it implements the entity reference selection plugin it inherently includes the ability to select bundles. Also, the MR in the other issue has a few advantages. It's a more complete patch. It's consistent with the way that the EntityReferenceItem class works instead of being a unique implementation. And it has tests.

Credit has been granted to everyone who worked on the patch in this issue. Thank you for your contributions!

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

Credit has been granted to those who worked on the patch.

🇺🇸United States dcam

I'm closing this in favor of Allow to select reference method in link field type for internal links RTBC . The other issue isn't specifically about adding the match_limit, but it is included in the MR. That MR is also way more complete than this patch, covering configuration updates, testing, and backward compatibility. It's also worthwhile to note that it has been RTBC twice already.

🇺🇸United States dcam

I just realized this is missing a change record. I'm setting it to needs work for that.

🇺🇸United States dcam

This looks great. I can't find anything to give feedback about.

The /admin/appearance page still works after the change. I also tried modifying a controller to test it for myself. I picked ContactController::contactSitePage(). The sitewide contact form continued to work after removing the class's constructor and moving the renderer argument to the controller function. I wanted to make absolutely certain that it was actually functioning as expected, so I tried breaking it by mangling the type hint and got an exception because a service with the mangled type hint couldn't be found. So I knew that it really was picking up the new argument and automatically injecting the service.

For any other reviewers, if you want to compare Drupal's new ServiceValueResolver to Symfony's, you'll find that they're quite different. See the explanation in #4.

I'm going to RTBC this. We'll see what the committers say.

🇺🇸United States dcam

I reviewed the new change. It looks fine.

All feedback has been resolved. I double-checked all the changes too. They look ok to me. I'm going to mark this as RTBC.

🇺🇸United States dcam

The changes for @xjm's feedback have been addressed with the exception of the one about the $cachedDiscoveries property. I left a suggestion for it.

🇺🇸United States dcam

I agreed with the idea of using the procedural service call in HistoryController. So I'm setting this back to Needs Work because of this.

Aside from that, it looks good to me. I could get nit-picky about code style in HistoryTokensHooks::tokens(), but I won't. I know it's because the code was copied faithfully from Comment. And I think it's beneficial for reviewers because it's easy to tell that there's nothing missing.

🇺🇸United States dcam

I reviewed the changes and they're looking good so far. I double-checked the #[Autowire(...)] attributes. They're all necessary from what I can see.

When I worked on the plugin override removal issue I asked PHPStorm to find all of the overrides for me instead of using grep. So I did that here in order to catch any child classes that extend from another intermediate base class. I only found one that looks like it can be autowired: core/modules/image/src/Controller/ImageStyleDownloadController.php.

I also found core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php, which looks like it ought to be autowire-able. It extends directly from ControllerBase, so either I'm wrong about the autowiring or maybe it got missed in the grep results.

🇺🇸United States dcam

All feedback has been addressed. I was concerned about the lack of full stops (periods) at the end of a couple of lines, but after doing a little research I decided not to hold this up on that point. There doesn't seem to be a consensus in the Core docs about whether a full stop should come after a URL and this isn't the place to go into that.

I checked for other instances of that old URL with grep -r "https://www.drupal.org/docs/8/theming" ./ and found none. All of the URLs updated by the MR go to the correct pages. There's no more out-of-scope changes. This looks RTBC to me.

🇺🇸United States dcam

I tried to be thorough about checking for other contact-related tests. There's probably a more efficient way to do it, but I used this grep command:
grep -ir --exclude-dir=contact --exclude-dir=migrate_drupal_ui --exclude-dir=phpstan-tmp --exclude-dir=fixtures --exclude-dir=lib --exclude=.phpstan-baseline.php "contact" ./. Here is what I found:

  • core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNullStorageTest.php - This isn't specifically contact-related, but contact is the only thing tested by it.
  • core/modules/config_translation/tests/src/Functional/ConfigTranslationUiModulesTest.php - It looks like testContactConfigEntityTranslation() needs to be copied.
  • core/modules/config_translation/tests/src/Functional/ConfigTranslationListUiTest.php - It looks like doContactFormsListTest() needs to be copied.
  • core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserRoleTest.php and core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserRoleTest.php - These test migrating permissions, including contact's. Does that need to be preserved?
  • core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserContactSettingsTest.php - Test migrating user contact settings from D6.
  • core/modules/user/tests/src/Kernel/Plugin/migrate/source/d7/UserTest.php - I may be mistaken, but I think this tests migrating user contact settings from D7.

Are these in-scope or is this issue limited to only migrate_drupal_ui tests?

🇺🇸United States dcam

@berdir I see your point. So would it be sufficient to do something like this?

public function __get(string $name) {
  if ($name == 'formBuilder') {
    @trigger_error(...);
    return ($this->formBuilder)();
  }
  elseif ($name == 'searchPageRepository') {
    @trigger_error(...);
    return ($this->searchPageRepository)();
  }
}
🇺🇸United States dcam

Oh, the questions I had while reviewing were answered. So I'll go ahead and RTBC this. The code looks OK to me. This service closure pattern has me thinking about where it can be applied elsewhere.

🇺🇸United States dcam

There needs to be a separate issue to support that.

Ok, maybe I should go ahead and drop it from 📌 Remove manual create() method from plugins Active so we don't have an inevitable merge conflict.

For the class's constructor arguments deprecation? I'm not sure how that would go.

Yeah, IDK either. Who constructs instances of plugins manually? But I've had to provide examples for such things in CRs like:

Before:

$block = new SearchBlock(
  $configuration,
  $plugin_id,
  $plugin_definition,
  \Drupal::service('form_builder'),
  \Drupal::service('search.search_page_repository'),
);

After:

$block = new SearchBlock(
  $configuration,
  $plugin_id,
  $plugin_definition,
  static fn(): FormBuilderInterface => \Drupal::service('form_builder'),
  static fn(): SearchPageRepositoryInterface => \Drupal::service('search.search_page_repository'),
);

You can use that if someone insists on it being in the CR.

🇺🇸United States dcam

This plugin is one of those having its create() function removed by 📌 Remove manual create() method from plugins Active in favor of autowiring. How do these issues affect each other?

Is it possible to add the #[AutowireServiceClosure(...)] attribute and delete create() here? In which case I'd remove it from #3552110.

Or do the parameter deprecations and need to handle either the closures or original parameters negate the possibility of using autowiring?

Also, does the CR need before and after examples?

🇺🇸United States dcam

I realized that we need to deprecate LinkWidget::validateTitleElement() instead of deleting it.

🇺🇸United States dcam

I performed the manual check as described in the issue summary on my suggestions. The suggestions also passed linting. So I pushed them here.

🇺🇸United States dcam

Looking good. I left a couple of suggestions.

🇺🇸United States dcam

How are you finding issues that are missing these attributes?

Brute force. There were only 111 RTBC issues this morning. I opened them all and skimmed their MRs. It took maybe 20 minutes.

🇺🇸United States dcam

Oh, really? It looked like core/modules/system/tests/src/Kernel/Theme/ThemeEngineTest.php was brand-new. My mistake.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

I added my own suggestion that was mentioned in #40.

🇺🇸United States dcam

I'm removing the change to the "The @uri field is required when the @title field is specified" validation from the issue. I tried writing a constraint validator for it, but it didn't work. The reason why is because the isEmpty() validation kicks in before the constraint validation. So if the URI is empty and the title is not, then Drupal simply thinks the whole field is empty and never checks any constraints. As far as I can tell, this has to remain as a form-level validation.

🇺🇸United States dcam

Rebased and updated the IS.

🇺🇸United States dcam
🇺🇸United States dcam

dcam changed the visibility of the branch 2839344-broken-aria-describedby-in to hidden.

🇺🇸United States dcam

Postponing on 🐛 Check of $value not covering LinkItem cases in link contraint validators Needs review which makes the same change to check field values are LinkItemInterfaces.

🇺🇸United States dcam

That's what I would do.

🇺🇸United States dcam

Of the 2 options it seems like MR 7117 would be the most flexible correct?

Let me be blunt: MR 7117 is nothing more than a workaround for a problem caused by irresponsible copying of parent element attributes to the children. MR 12287 fixes the problem at its source. I feel strongly that it's the way to go, so much that I've considered simply closing 7117. But I left it open to be fair to the others who have contributed here. And BTW 12287 has a test.

If there is interest in having the isComposite(), then that can go into a follow-up issue as a feature request. Let's fix this bug properly instead.

🇺🇸United States dcam
🇺🇸United States dcam

This is unblocked.

🇺🇸United States dcam

This is unblocked and needs a rebase to integrate the new test that was added by both this issue and the blocker.

🇺🇸United States dcam

Any reason why Drupal\image\ImageEffectBase is not converted?

I checked for logger channels that are defined and not in use by the plugins, but missed that one. Thanks for catching it.

🇺🇸United States dcam

Thank you for working on this issue! I've left some feedback in comments on the MR.

🇺🇸United States dcam

Good thinking about the trait. I hadn't considered that.

My feedback was addressed and the new changes to check traits look good. I'll RTBC this.

🇺🇸United States dcam

I verified that the new regex is the same that went into Coder. Then I tested PHPCS on one of the double-stops that's being patched here. Here are the results:

~/drupal_core$ phpcs core/tests/Drupal/Tests/Core/Htmx/

FILE: /var/www/html/core/tests/Drupal/Tests/Core/Htmx/HtmxUtilitiesTest.php
-----------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------
 113 | ERROR | [x] Documentation comment contains forbidden comment "Test ::applyTo with attribute key..".
-----------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------

This looks good to me.

🇺🇸United States dcam

I didn't find anything to give feedback about. I tested the rule by adding a disallowed annotation to a method and it was picked up by PHPStan:

~/drupal_core$ phpstan core/modules/link/tests/src/Unit
 7/7 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------------------------------------
  Line   Plugin/Validation/Constraint/LinkAccessConstraintValidatorTest.php
 ------ --------------------------------------------------------------------
  28     Test method testValidate must not add annotation @covers.
         🪪 testMethod.metadataForbidden
 ------ --------------------------------------------------------------------

I tried it out with some of the other forbidden annotations too and got the same result. An allowed annotation did not generate an error. So it's looking good to me.

I left one small grammar nitpick in an MR comment. You can ignore or apply as you see fit.

🇺🇸United States dcam

I was kind of confused by the removal of the text-with-summary fields from the D7 fixture. After all, it's not like the field is being removed from D7. But I guess you have to do that because we wouldn't want to change the migration plugins to migrate D7 text_with_summary fields to text_long, right?

I double-checked the MR. I can't find anything else to give feedback about. So I'm going to RTBC it.

🇺🇸United States dcam

Adding a permission for it seems like a good idea to me. We're heavily dependent on Layout Builder at work. If it weren't for revisioning that button would really scare me because of the content editors' access. But for the record, 📌 Improve clarity of Layout Builder "Revert to defaults" confirmation form Active was committed a couple of weeks ago that at least warns the user about what they're doing.

🇺🇸United States dcam

Or what about 'entity title' vs content

Except you can only reference nodes.

All the patches were just hidden...

@philltran When we create MRs on issues that have been around since the old patch days we hide the old patch files. I assume this is done mostly for the sake of de-cluttering the issue summary. And there are probably other benefits like making sure people are focusing on the right thing when working or reviewing. But if you have a new patch that was made for the sake of helping people who need a stable fix for something, then that's relevant content and you shouldn't worry about doing it.

🇺🇸United States dcam

With regards to my concern if no one else has it I'll ignore it.

I'm biased because it's been directly impacting me. Maybe we need to tag it for framework or even release manager review.

🇺🇸United States dcam

...not 100% sure 'content title' is correct. What if I'm linking to a media or something else is that still "content"

I don't mind changing it. I don't love how it's worded anyway. But how do we say tell people to enter the title of a node when we aren't supposed to say "node" in the UI?

🇺🇸United States dcam

I've seen that random Unit test error twice before on another issue. It's unrelated.

🇺🇸United States dcam

dcam changed the visibility of the branch 2828556-11_2_x to hidden.

🇺🇸United States dcam

@philltran most people generate a patch file for the version and upload it to the issue rather than opening a new MR. There's nothing wrong with creating a new branch for that purpose, but as you've seen creating an MR from that branch results in confusion.

I'm going to hide your 11.2 branch in the issue UI so that it's clear which MR needs to be reviewed. This won't make your branch go away permanently. There's a link to show hidden branches below the branch list. So you can maintain it and keep it up to date if necessary.

🇺🇸United States dcam

I responded to the feedback that was already given.

🇺🇸United States dcam

@mstrelan I'm still working on it, finding issues caused by removing too much.

🇺🇸United States dcam

Not sure what the best way is either, but PHPStorm located overrides of PluginBase::create() for me and I'm going with that.

🇺🇸United States dcam

dcam made their first commit to this issue’s fork.

🇺🇸United States dcam

I left some comments on the MR.

I noticed some assertions that were migrated to Text module stats that don't seem to be necessary. For instance, TextWithSummaryMigrateFieldFormatterSettingsTest seems to have a lot. Only 5 of the assertions were removed from the original MigrateFieldFormatterSettingsTest. I couldn't figure out if these assertions that aren't related to text-with-summary fields are necessary or not. Was their inclusion intentional?

🇺🇸United States dcam

I started working on this, but realized it's postponed on 🐛 Invalid path error message is displayed three times in link widget for external link Closed: cannot reproduce because we need the fix for inline form errors to be integrated into the validators.

🇺🇸United States dcam

I'm prioritizing this over 🐛 Check of $value not covering LinkItem cases in link contraint validators Needs review because that one's no longer RTBC. This MR has the more complete LinkTypeConstraintValidatorTest. And IMO it's the more serious bug.

🇺🇸United States dcam

Since I made changes I've decided to postpone this on 🐛 Invalid path error message is displayed three times in link widget for external link Closed: cannot reproduce instead of the other way around like we've been doing. The other issue has a more complete LinkTypeConstraintValidatorTest test class. I'd also consider the other issue to be the more serious bug since it can happen simply by enabling Inline Form Errors whereas this one seems like an edge case potentially requiring custom code.

🇺🇸United States dcam

I wasn't happy with the validators returning if the input wasn't a LinkItemInterface. So I looked up the Symfony validator docs. They show an UnexpectedValueException being thrown in instances just like what we're dealing with. So I'm taking that as a best practice.

🇺🇸United States dcam

Feedback was addressed.

🇺🇸United States dcam

Feedback was addressed.

🇺🇸United States dcam

I reviewed the last commit that disabled PHPCS around TestWith attributes. That seems like a reasonable approach to me until coding standards are developed. Should a follow-up issue be created for it?

🇺🇸United States dcam

I left a comment on the MR.

🇺🇸United States dcam

The changes added on September 17 to add comments for the HTMX test functions look good to me. I think this is just about ready. I only found one indentation nitpick in my review.

🇺🇸United States dcam

I can't find anything to give feedback about. It's a pretty straightforward change. Still, I tested the form thoroughly. The system.date configuration continues to update appropriately. Seems RTBC to me.

🇺🇸United States dcam

Looks good. Let's do it.

🇺🇸United States dcam

Good grief, what a pain! I kind of can't believe such a breaking decision was made upstream by PHPUnit.

I read through the entire nearly 30,000 line patch. It's just the same two changes over and over and over... The only strange things about it were the three instances of alphabetized unrelated use statements.

I was going to say "Let's get this in before it gets stale," but it already is and needs a rebase.

🇺🇸United States dcam

Feedback has been addressed. The additional changes to qualifying MockObjects look good to me too. And the tests are green. So I'm marking this one as RTBC.

🇺🇸United States dcam

I left one question on the MR, but I didn't do a complete review because I realized how late it is and how tired I am. So I'm leaving the status at NR.

🇺🇸United States dcam

The current changes look good to me. Thank you for the debugging and explanation of the ResolvedLibraryDefinitionsFilesMatchTest change.

The only thing I noticed is that item 6 of the Remaining Tasks is to deprecate libraries only used by the module. The module has one library that's only used by itself. Does that need to be deprecated before this can be RTBC or does that have to wait since stable9 is overriding it? I don't see any discussion about it.

Production build 0.71.5 2024