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.
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.
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.
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.
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.
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.
It sounds like this should be postponed on #3152587: Add a mark when editing a field property will affect all the translations as we do with fields themselves → .
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.
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.
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!
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.
I just realized this is missing a change record. I'm setting it to needs work for that.
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.
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.
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.
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.
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.
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.
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 liketestContactConfigEntityTranslation()needs to be copied.core/modules/config_translation/tests/src/Functional/ConfigTranslationListUiTest.php- It looks likedoContactFormsListTest()needs to be copied.core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserRoleTest.phpandcore/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?
@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)();
}
}
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.
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.
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?
I realized that we need to deprecate LinkWidget::validateTitleElement() instead of deleting it.
I performed the manual check as described in the issue summary on my suggestions. The suggestions also passed linting. So I pushed them here.
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.
Oh, really? It looked like core/modules/system/tests/src/Kernel/Theme/ThemeEngineTest.php was brand-new. My mistake.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
dcam → changed the visibility of the branch 2839344-broken-aria-describedby-in to hidden.
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.
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.
This is unblocked and needs a rebase to integrate the new test that was added by both this issue and the blocker.
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.
Thank you for working on this issue! I've left some feedback in comments on the MR.
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.
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.
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.
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.
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.
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.
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.
...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?
I've seen that random Unit test error twice before on another issue. It's unrelated.
@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.
@mstrelan I'm still working on it, finding issues caused by removing too much.
Not sure what the best way is either, but PHPStorm located overrides of PluginBase::create() for me and I'm going with that.
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?
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.
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.
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.
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.
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?
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.
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.
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.
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.
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.
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.