Problem/Motivation
#2054011: Implement built-in support for internal URLs added an improved link field type and corresponding widget that allows to reference internal and external paths.
Shortcut still has (pretty hacky) separate fields for that.
Also, this avoids the problem with computed field validation which we have with current hacky fields. Thus, this conversion is critical as it blocks #2403847: Shortcut content entity validation misses form validation logic.
Proposed resolution
Just like menu links will in #1842858: [PP-1] Convert menu links to the new Entity Field API, we should make shortcut entities use that field to simplify the UI code and fields it needs to use.
This also serves as a real-use test for converting menu links.
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Update the issue summary | Instructions | ||
Update the issue summary noting if allowed during the beta | Instructions |
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#136 | 2235457-135-followup.patch | 587 bytes | hussainweb |
#127 | 2235457-127.patch | 30.84 KB | dawehner |
#127 | interdiff.txt | 3.57 KB | dawehner |
#122 | 2235457-122.patch | 29.14 KB | dawehner |
#118 | interdiff.txt | 1.01 KB | dawehner |
Comments
Comment #1
BerdirYeah, the link field is as fun as it is broken :)
Known issues:
- Edit seems broken, the value is empty, the shortcut being edited is then also missing from the list
- Translatability, I guess we need to integrate the link field type with entity translation sync so that we can make the title translatable?
- Or should we not use the title of the link field?
- Currently set to type general, using internal only would simplify a lot and pretty sure we want that, but didn't want to think about that yet, just applying patterns that I found elsewhere..
- unserialize is ugly, but I have no idea how we can have passing tests right now, probably because we only use it as configurable field, which gets serialization handled properly?
Comment #3
amateescu CreditAttribution: amateescu commentedMost of the brokenness you found was actually because you forgot to put a 'link__url' column in the schema ;) Another part of it was coming from the change in #2054011-70: Implement built-in support for internal URLs, because
massageFormValues()
doesn't run on programatic updates. I had to revert that hunk and write a proper fix.I'd prefer to not use the title property of the link field, mostly because I see it as a main property of the entity.
shortcut.module goes to great lengths to ensure that the links are internal only (@see
shortcut_valid_link()
), so we definitely want the internal links option.Yup :/
Here's an updated patch that will fix most of the errors. The patch above didn't apply, so the interdiffs for shortcut.install and ShortcutFormController are not reliable.
That said, we should have only two relevant failures left:
- shortcut supported an empty path as the url by converting it automatically to the front page. We need to either bake that into the link field or drop this feature.
- the link field doesn't support aliases atm, that's being fixed in #2231413: Link field does not accept a valid path alias
Comment #4
amateescu CreditAttribution: amateescu commentedRemoved some debug leftovers and updated the label for this field.
Comment #5
Berdir- __url schema => ups :)
- Are sure that preSave() doesn't mess with re-saving shortcuts without changes? That's why we moved the code there to massageFormValues(), but if that test still passes then that's fine with me.
I think menu links always supported that by using as the path, I'd be fine with removing that special feature.
Do we have test for deleting shortcuts? That was broken when I was manually testing it and is related to #2232321: Limit validation errors on confirm form.
Comment #6
amateescu CreditAttribution: amateescu commentedI ran that test locally and it seems there's still a problem. I'll try to figure out a better solution because, unfortunately, moving that code to massageFormValues() is not the right fix..
Not sure if we have a test for deleting shortcuts but a manual test worked.
Comment #8
amateescu CreditAttribution: amateescu commentedOk, so the logic of the fix was correct, just the implementation was wrong. We should be left only with #2231413: Link field does not accept a valid path alias now.
Comment #10
BerdirI'm not sure if this really makes sense here, is url really the main property and not just one of a set of equally important ones?
What's interesting is I can't find a single use of this information right now in core, #2182239: Improve ContentEntityBase::id() for better DX will add a usage though
Seems like you just use it in one place in the test, where you know it's a shortcut, so you could just hardcode url there if that's what you want?
This can be simplified as well with only internal URL's...
We have to do this in two places now, and unfortunately it's not the same, one is just an array and the other is a render array, but still wondering if a method on Shortcut that returns the render array for it would be useful?
I know you were considering to use view builders for menu links, but I think that is too much overhead, considering all the render cache logic that implies and that rendering an entity involves 2 hooks and two alter hooks at least. Or maybe an interface for a common set of methods that both shortcut and menu link will have so you can use the same code or maybe a helper method to do it?
Do we want to keep the test but use ? Might need some special casing somewhere...
Either way, this looks pretty awesome now, just a bit worried that the field type change will imply yet another large-scale change for all our menu links... maybe we should allow to pass in the current structure for those, to avoid changing all default menu link definitions? Not worth for shortcuts...
Changing to major, as this also fixes a bunch of problems in the link field type that we want to have to make it easier to use elsewhere.. (redirect currently has the unserialize hacks in the entity class)
Comment #11
amateescu CreditAttribution: amateescu commentedI assume you meant
<front>
there, and yes, that's a good point. Fixed and it didn't need any special casing.About menu links, I have no idea anymore how those will look like in D8, see #2178723: [meta-2] Finalize module-facing API of declaring menu links and the fourth child issue, so I prefer to not speculate on it at this point..
Comment #12
BerdirYes, I was just thinking out loud about default menu links, obviously not something we need to worry about here.
The other things were just ideas/questions, nothing blocking, so I think this is ready to go. I provided the initially patch but I think everything there has been reviewed and improved by @amateescu, so there's hardly any code left from the initial approach which was just a proof of concept :)
Comment #13
amateescu CreditAttribution: amateescu commentedSadly, we still depend on #2231413: Link field does not accept a valid path alias :(
Comment #14
BerdirOh, stupid me :)
Comment #16
amateescu CreditAttribution: amateescu commentedComment #17
YesCT CreditAttribution: YesCT commentedComment #18
Berdir11: shortcut-link-2235457-11.patch queued for re-testing.
Comment #20
Jalandhar CreditAttribution: Jalandhar commentedJust re-rolled.
Comment #22
amateescu CreditAttribution: amateescu commentedThis should be better. Unfortunately, #2231413: Link field does not accept a valid path alias fixed the path alias problem at the wrong level...
Comment #24
amateescu CreditAttribution: amateescu commentedMeh!
Comment #25
BerdirLooks good, two questions, a) Can/Should we remove the getSystemPath() call from where the other issue added it? and b) Some explicit test coverage would be nice, for example in the unit test?
Comment #26
amateescu CreditAttribution: amateescu commenteda) It's already removed in the patch, see
LinkWidget::massageFormValues()
:)b) Sure, somethis like this?
Comment #27
YesCT CreditAttribution: YesCT commentedupdated issue summary to reflect this is nolonger blocked on #2231413: Link field does not accept a valid path alias
tried to identify remaining tasks. (please correct them, if I missed something)
Comment #28
Berdira) Ah, wasn't visible in the interdiff because.
b) Yes, looks fine :)
@YesCT: No remaining steps :)
Comment #29
chx CreditAttribution: chx commentedI think ultimike's #2242779: Link Module - Add an optionless link test test (maybe just as a new method to the existing test) should be merged into this because of the change in link module is untested.
Comment #30
amateescu CreditAttribution: amateescu commentedI looked at the patch from #2242779: Link Module - Add an optionless link test and the new test seems to be a verbatim copy of
Drupal\link\Tests\LinkItemTest
with some assertions commented out. What exactly is there to bring into this patch?Comment #31
YesCT CreditAttribution: YesCT commentedIn addition to not doing some assertions,
it also commented out, in the test, setting of some options.
I think in that issue, we were hoping it would fail, to show that something needed to be fixed.
Comment #32
amateescu CreditAttribution: amateescu commentedOk, so we don't have anything to bring in here?
Comment #33
ultimikeThe test that chx referred to in comment 29 isn't done yet - my thinking is that we need a test that saves a new link using $entity->save() (not Form API) and then tests the rendering of the link (not just testing entity_load()).
-mike
Comment #34
webchickNo longer applies.
Comment #35
lokapujyaStraight reroll. A deleted file from the patch was renamed in another patch.
Comment #36
lokapujyaComment #37
chx CreditAttribution: chx commentedThis is fine; probably needs a followup for testing the link cleanup; but that shouldnt be holding up a major task.
Comment #39
BerdirConflicted in ShortcutInterface.php, git apply -3 patch worked fine.
Comment #40
catchIn the vast majority of cases, this should already be using the system path. Should check $this->options['alias'] first. Otherwise this will add a lot of unnecessary queries.
If the caller doesn't know this, it should do the check, not Url itself.
Didn't review past that line.
Comment #41
lokapujyaWithout that line, creating a shortcut to an alias fails testing.
Comment #42
Wim Leers39: 2235457-39.patch queued for re-testing.
Comment #44
lokapujyagetSystemPath() changed to getPathByAlias() in Core/Url.php.
Comment #46
dawehnerFixed the failure + added some documentation.
It is kind of sad that we have to do that, but yeah path alias handling is done on the request level. Should we maybe document that here?
Is there really a need to store the URL, given that shortcuts will always link to internal paths.
As written before, it would be nice to not require both.
Comment #47
Wim Leers#46: did you forget to upload the patch? :)
Comment #48
Wim Leers@dawehner, you forgot to upload the patch AFAICT. :)
Comment #49
amateescu CreditAttribution: amateescu commentedRefactored the code a bit so we don't need to store or use the 'url' property when we're dealing with internal paths only, but, sadly, this means we're now blocked on #2232321: Limit validation errors on confirm form.
@catch: We don't have any
$this->options
there becauseUrl::createFromPath()
is a factory method that receives only one argument, a path string.I did see the performance problem when I added that line but we don't really have a good way to fix it, because making the caller always ensure it's sending a system path would make it quite easy to overlook (like we (I) did when adding internal path support to the link field).
One way of making it better (perf wise) would be to try and resolve the alias only if the initial match didn't yield any result. That would mean something like this, query wise:
- if a system path was passed in => the initial match finds it => 1 query
- if an invalid alias was passed in => initial match fails, resolving the alias fails => 2 queries
- if a valid alias was passed in => initial match fails, alias is resolved, second match finds it => 3 queries
That's quite a pessimistic scenario so any other ideas are welcome :)
Comment #50
amateescu CreditAttribution: amateescu commentedAs suggested by @Berdir in #2232321: Limit validation errors on confirm form, let's just merge that patch in here.
Comment #52
anavarreComment #53
Anonymous (not verified) CreditAttribution: Anonymous commentedAttaching my attempt to re-roll this for PSR-4 following the instructions here: https://groups.drupal.org/node/424758
Result was "Fatal error: Unsupported operand types in /var/www/drupal/core/lib/Drupal/Core/Routing/UrlGenerator.php on line 242" when I ran a migration on a fresh install.
This is currently blocking #2277695: D6 > D8 link migration testing for me. I also tried to re-roll #2242779: Link Module - Add an optionless link test as an alternative, but this produced the same result.
Comment #54
lokapujyaComment #56
BerdirThe patch accidentally re-added the manual schema and the path field, removing that makes it install again, did not check the patch beyound that yet.
Comment #58
BerdirComment #62
benjy CreditAttribution: benjy commentedJust a straight re-roll to try bring this issue back to life.
Comment #64
benjy CreditAttribution: benjy commentedI was going to take a look at this but after the amount of effort it took to re-roll i'm just posting the patch to see what else might have broken. I'd be happy if someone a little more qualified in this area could pick the issue up.
Comment #66
benjy CreditAttribution: benjy commentedGit!
Comment #68
dawehnerSorry but these changes are certainly wrong. We explicit introduced the path validator to handle the usecases here. In case it doesn't it has to be adapted.
Did we considered to add a getUrl method directly on the link item?
Comment #69
dawehnerRerolled, let's see how much fails.
Comment #71
mgiffordComment #72
dawehnerLet's reroll and get it installed.
Comment #74
dawehnerLet's see.
Comment #76
dawehnerSome work ...
Comment #77
chx CreditAttribution: chx commentedComment #79
dawehnerA bit less ...
Comment #80
dawehnerWith an interdiff ...
Comment #82
jibranNumber of times @dawehner clear the cache in tests to make em green is to damn high.
Comment #83
dawehnerThe amount of wasted time without doing patch reviews is crazy.
Let's fix some of the failures.
Comment #85
yched CreditAttribution: yched commentedMostly reviewed the link.module part for now.
Nothing I'm familiar with, but this could warrant a comment explaining why 'base://' deserves special consideration ?
It feels very weird that we need to clear the discovered widget plugins - and especially that often ?
What causes the set of available widgets to change here ?
Hmm - such code is totally correct in the widget's massageFormValues(), but doesn't feel right here in LinkItem::preSave() :
I don't think we want to support "$link_item->url can be either a raw url, or the 'path' part of a parsed url with ->options containing the parsed options", it should be one or the other ?
(and thus it has to be the latter, since this is what the widget produces and what gets stored and loaded)
I don't think allowing polymorphism on "what is found in an Item properties" is a good thing, since it means you de facto don't know what's in there and what you can do with the values at runtime.
If needed, we can always add a $link_item->setPath($raw_url) as a helper for DX, that immediately expands into the single canonical shape for the url and options properties.
'0' is moot for either url or root_name, right ?
couldn't it be just
return empty($this->route_name) && empty($this->url);
?It should indeed - I thought we had support for 'serialized' columns in the field schemas (I think the image field type also has a serialized column (sorry, can't check atm), would be worth checking how it's done there ?)
And if not, we should totally open a followup and link it there.
Hm - that, on the other hand, might belong to LinkItem::preSave() ?
Or is accepting aliases as input is only a feature of the widget, and is not supported on runtime LinkItem value ? Yeah, could make sense I guess.
Anyway - again, it's a question of having clear and unambiguous semantics for the properties in a runtime LinkItem object.
Comment #86
dawehnerThanks @yched for the review, though this is for now just a reroll.
Comment #87
mgiffordI just want to engage the bot on this. Need to still address @yched points.
Comment #89
dawehnerLet's do just the reroll.
Comment #91
YesCT CreditAttribution: YesCT commentedneeds a beta evaluation (especially since this is a task)
Comment #92
andypostAt least related #2315773: Create a menu link field type/widget/formatter
Comment #93
YesCT CreditAttribution: YesCT commenteddoes this issue close #2277753: Shortcuts should handle missing routes elegantly ?
Comment #94
pwolanin CreditAttribution: pwolanin commentedWe should possibly change course based on #2407505: [meta] Finalize the menu links (and other user-entered paths) system
At least, for shorcut we want to store the route info, for link field just the user-entered path
Comment #95
larowlanIf we just want route info, then do we want a PathElement based widget?
Takes path, converts to route/route name pair?
Comment #96
catchThere's been more discussion on #2407505: [meta] Finalize the menu links (and other user-entered paths) system.
Once the link field is applied to menu links, it makes sense to apply it to shortcuts too. This reverses the path vs. route storage decision but means we have the same storage model and expectations everywhere.
Tagging D8 upgrade path since this will require a schema change. Undecided on whether this is critical or not so leaving at major for now. If we don't treat it as critical, we need to discuss whether we'd go ahead with it after the upgrade path is supported with a hook_update_N() - mostly trying to avoid that though.
Comment #97
amateescu CreditAttribution: amateescu commentedAnd sanity is restored! Nothing more I could wish for :)
Comment #98
larowlanI think this now blocks #2403847: Shortcut content entity validation misses form validation logic which is critical - so does that in turn make this critical? or do we have enough criticals tracking this space.
Comment #99
fagoEnough yes, but afaik that's the way it works :/
Comment #100
fagoUpdated summary.
Comment #101
dawehnerLet's wait until #2411143: Change LinkItem schema to store URIs rather than URLs/paths/routes is in ... these potentially fixes a lot of the problems we had earlier here with various route related issues.
Comment #102
dawehnerUnpostpone it.
Currently working on a "reroll", let's see how far we can get.
Comment #103
dawehnerHere is a first version.
One question I came up with: Where should the title of a shortcut be stored ... does that belong onto the link or not.
Comment #105
dawehnerFixed the failures.
Comment #107
hussainwebAttempting to fix one of the failures, just so that I understand this better.
Comment #109
hussainwebThis will not fix the last failure but I am just changing a test and I thought it best to do that independently. I will attach the patch with the fix shortly after this. I will set it to Needs Review with that patch.
Comment #110
hussainwebThis should fix the last two failures, but it is not done yet. The fix uses the Url::getInternalPath() method which is deprecated. The trouble is I can't find any substitute for this method except of inlining the function code (which might be wasteful in a test, especially if there is a ready substitute method).
We could also let this go in and let the code that removes the deprecated calls handle this. It is already used in over a dozen places.
Any suggestions?
Comment #111
hussainwebAh, all green.
Going up the comments, I see that the last review comments appeared in #85, but I am not sure if that is still valid. @yched, can you check again please? I know that at least one of the review comments were addressed in one of the patches since then.
Comment #112
RavindraSingh CreditAttribution: RavindraSingh commentedAwesome,
but the Url::getInternalPath() is needs to be removed as deprecated function. So this can be achieved in similar way.
\$shortcut->getUrl()->getPathFromRoute($shortcut->getRouteName(), $shortcut->getRouteParameters()), '/'))\
Can be used as substitute.
Comment #113
RavindraSingh CreditAttribution: RavindraSingh commentedComment #115
hussainweb@RavindraSingh: Can you give an interdiff for your patch? Is that the only line you have changed?
Comment #116
hussainweb@RavindraSingh: I did mention about this deprecated method in #110. I agree that it should be removed but I don't know if it is a good idea to block a critical on it. If we can fix it, great! But it doesn't seem that simple, not without adding many lines for this trivial use case.
Besides, there are lots and lots of places where this method is used. This could just be another instance to change when we deal with that issue. I am not convinced that the snippet you have given is actually the best way to substitute this. I have put in all the places where this method is used in #2413701-1: Url::getInternalPath() is marked deprecated but won't be removed for 8.0.x, so update the docs.
I would suggest that if we can quickly figure out a good substitute, we change it. Otherwise, we worry about the substitute in #2413701: Url::getInternalPath() is marked deprecated but won't be removed for 8.0.x, so update the docs and not block this critical. I still suggest the patch in #110. If everyone agrees, I will reupload it.
Comment #117
hussainwebI was trying out the snippet in #112 and realized that the method getPathFromRoute() is also deprecated. This means that if we can't use getInternalPath(), we can't use getPathFromRoute() either.
I am reuploading the patch from #110 to keep things clear until we find a better substitute. Else, I still suggest in dealing with that in #2413701: Url::getInternalPath() is marked deprecated but won't be removed for 8.0.x, so update the docs.
Comment #118
dawehnerLet's keep it simple :)
Comment #119
hussainwebHahaha... Never realized those were public. :)
Comment #120
dawehner@hussainweb
Really good catch in https://www.drupal.org/files/issues/interdiff-107-109_0.txt
Comment #121
Wim LeersThis patch is absolutely beautiful. If I were emotionally attached to Shortcut module, I'd cry!
I only found nitpicks, hence keeping at NR.
I think we can just keep the last plugin cache clearing call?
This still needs to happen in this issue?
Is this a new annotation thing? Have not seen this before.
These are unnecessary changes; they don't actually change anything, they just reformat existing code.
Fine to keep though.
Comment #122
dawehnerNot sure whether this was maybe just some sort of desperation.
Opened an issue for that. A new critical issue in entity storage is a good start into the day: #2414835: SqlContentEntityStorage does not unserialize base field data
It is IMHO the best thing since sliced bread. It adds DX, because it explains, which properties exists, instead of of having nothing.
Comment #123
Wim Leers:D
Manually tested, still works fine. RTBC!
Two simple whitespace fixes in this reroll.
Comment #124
Wim Leersdawehner had an excellent remark in IRC:
Plus amateescu's comment in #3:
IOW: anything that is a semantically essential field of an entity, should be an actual base field itself. Hence keeping this at RTBC.
Comment #125
amateescu CreditAttribution: amateescu commentedOverride :)
Based on the discussion above, this seems wrong.
I don't see where we remove this class in the patch. Same for ShortcutPathValue.
We should assert more fields/properties like before, not just ->link->options :)
Afaik, messages that might be translated in the UI are also asserted with t().
Shouldn't the default be an empty array? Why do we need to set it specifically?
Here we rely on mainPropertyName(), how about passing an array with the 'uri' key?
Comment #126
catchComment #127
dawehnerGood points!
Comment #128
amateescu CreditAttribution: amateescu commentedNote for other reviewers: #125.3 is not in the interdiff but it is included in the patch.
I worked a lot on this patch many months ago but I think it received enough updates and reviews from other people so setting back to RTBC :)
Comment #129
dawehnerDamn, I suck at git.
Comment #131
Gábor HojtsyIt was discussed above that even though the link field has a title (and a description following #2413029: Change LinkItem schema to also store a description), that is not being used because the entity needs a label. Why is the field kept translatable then? Do we expect to support links different per language in a shortcut? The similar menu link patch at #2406749: Use a link field for custom menu link does not make the link field translatable.
Comment #132
alexpottLess shortcut specialness - yay. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f2fa574 and pushed to 8.0.x. Thanks!
Comment #134
Gábor HojtsyI misread that. setTranslatable(FALSE) is fine. That is the default, so we don't do setting it to FALSE anywhere other than 2 tests, so it would be nice to remove.
Comment #135
Wim LeersBut the code already says
setTranslatable(FALSE)
— so the link field isn't translatable.Discussed with Gábor in chat.
setTranslatable(FALSE)
is the default. Hence it's opt-in, and he misread it precisely because it's opt-in: presence of asetTranslatable()
call at all typically implies it's translatable.So, rectifying that detail in #2415129: Small clean-up for Shortcut entity's usage of link field.
Comment #136
hussainwebI am uploading a patch to address #134 and setting that to Needs review. I know it is not critical and is already default, so wouldn't break anything, but it would be nice to keep this consistent.
Comment #137
Wim Leers#136: that's very helpful — thank you! — but we don't post follow-up patches after the patch for an issue has been committed. I've already filed a follow-up issue with the exact same patch, as you can see in #135 :)
Comment #138
hussainweb@Wim Leers: We must have submitted at the same time. I saw your comment once the page reloaded. :)
Comment #139
yched CreditAttribution: yched commentedLate to the party, but I could indeed cry at the amount of weirdness that was removed here :-)
I too was wondering why the Shortcut entity preserves a separate 'title' field and keeps the 'title' property in its LinkItem unused, but I see this has been discussed at length above. Just, since it seems a couple people raised the topic, maybe it would be worth writing down the reason in Shortcut::baseFieldDefinitions() ?
(also - Shortcut::label() could return $this->link->title, couldn't it ?)
Other than that :
I thought that was unintentional, then I saw #121 / #122 :
Hmm, why not on principle, but that indication is incorrect, $shortcut->link is a FieldItemList, not a LinkItem ?
Comment #140
BerdirIt could, but we commonly don't override that method and it would e.g. require a shortcut entity query selection plugin, if you'd want to support autocomplete. Users are already annoying enough in regards to that.
Agreed that it is technically the wrong class, there is afaik a separate issue that is trying to add this in a lot of places, and it worked around this by adding both the list and the item class *and* itemClass[] IIRC, so that it reflects magic __get/__set and array access.
Comment #141
amateescu CreditAttribution: amateescu commented@yched, the discussion here was not really at length, see #2406749-17: Use a link field for custom menu link and below if you want to join the fun.
Comment #142
amateescu CreditAttribution: amateescu commented@Berdir, or we could do the second to last paragraph from #2406749-25: Use a link field for custom menu link. Not saying that we have to, I'm just trying to lay down all the possible options.
Comment #143
BerdirAnd see #2336101: [policy, then patch] Add @property statements to FieldType classes for better IDE completion for the @property issue, which has been delayed forever by moving it to the Technical Working Group.
Comment #144
amateescu CreditAttribution: amateescu commentedWe kind of broke every D8 install in a way that I'm not sure is even fixable manually :( #2415633: Fatal error: Class 'Drupal\link\LinkItemInterface' not found in core/modules/shortcut/src/Entity/Shortcut.php on line 156
Comment #145
pwolanin CreditAttribution: pwolanin commentedI'd also like to see us use the associated Link field title column as long as translation will work - otherwise the link field vs shortvut and menu ink will potentially remain somewhat divergent.
Comment #146
Gábor Hojtsy@pwolanin: I believe that would need #2403455: FieldTranslationSynchronizer does not sync columns that are absent from any column_group in the annotation (which IMHO is not a problem) to be able to translate title/description but not the rest.
Comment #147
fagoInteresting to see
LinkTypeConstraint
being a single class, as suggested by #2226821: Combine Drupal Constraint and ConstraintValidation classes into one a while ago. However, looking at it I'm not sure it's something we should practice, let's discuss at #2226821: Combine Drupal Constraint and ConstraintValidation classes into oneComment #148
YesCT CreditAttribution: YesCT commentedpatch #1 here has code for default_value and max_length = 560 but that is dead code.
#2417809: link and shortcut have baseFieldDefinition settings that do not do anything: default_value max_length