Updated: Comment #64
Problem/Motivation
The contrib link modules Link, URL Field and CKEditor Link supports both internal and external URLs and a lot of people use the module for internal links. Right now, the storage for the core link field data and the default widget only supports external URLs.
Steps to reproduce the issue:
- Install Drupal 8.x
- Enable the field type 'Link' in /admin/modules
- Goto manage fields for one of the content types in /admin/structure/types
- Add a new field with existing field type 'Link' and save
- In edit tab scroll down to see the bug
Below is the attached screenshot :
Proposed resolution
Implement plan C, described below.
Updated: After screenshot implementing plan C (provided on #64)
Remaining tasks
None.
User interface changes
A new option appears in the link field instance edit page. See screenshot from comment #64.
API changes
- both internal and external URLs can be stored (and edited in the default widget) by the link field
- the field type plugin receives its own interface (LinkItemInterface), which contains an isExternal()
helper method
Original report by @Dave Reid
Current summary from #1778858: Decide on built-in support for internal URLs which was filed against the URL module issue queue:
Plan A:
- Decide we only want to support external URLs in the widget and change nothing. Leave internal URL widget for contrib.
- Pros: Lowest complexity
- Cons: Users may find out-of-the-box solution limiting.
Plan B:
- Rename the current widget from 'URL field' to 'External URL field' and add a new 'Internal URL field' widget.
- Pros: we support both, not too much extra code
- Cons: this makes a specific instance only able to use one or the other and not mix.
Plan C:
- Add an option on the field instance to have the administrator say if this field supports internal only, external only, or both. The default widget will change it's validation based on the options.
- Pros: Able to mix/match. Seems better as a field instance setting, not a widget setting.
- Cons: Most amount of complexity to add. Widget will not be able to support HTML5 'url' FAPI type if option is not 'external only' since the url field by nature only supports external fields.
Plan D:
- Always use the FAPI 'url' type, and attempt to resolve what are 'local' URLs to un-aliased internal Drupal URLs.
- Pros: Simplicity. We are able to handle both types with the same widget and use the new HTML5 FAPI type always.
- Cons: Medium complexity.
- How does this work for sites that have multiple domains (multisite or domain access) or SSL? Would we still want to add additional logic for 'Allow the following types of URLs: [X] Internal [X] External'?
Comment | File | Size | Author |
---|---|---|---|
#116 | interdiff.txt | 1.14 KB | dawehner |
#116 | internal_url-2054011-116.patch | 49.7 KB | dawehner |
#113 | interdiff.txt | 1.32 KB | dawehner |
#113 | internal_url-2054011-113.patch | 49.7 KB | dawehner |
#112 | interdiff.txt | 1.62 KB | dawehner |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedHere's a very good reason why we really need to do this: #2051889-7: Add route parameters property to menu links
Comment #1.0
klonos..link to the link module ;)
Comment #2
dawehner.
So to sump up the other issue:
Plan D
Comment #3
dawehner@amateescu
What are your plans to move this forward?
Personally the only option from A to D seems C for me. Given the problem that html5 validation is problematic anyway I think it is fine to drop support it for non-external URLs. As it uses the FAPI type url, you need to hack around for internal URLs in quite some places.
PS: Please ignore me previous comment ...
PS2: This is just a rough outline for the route/route_parameter part.... happy to discuss how to do it properly.
Comment #4
amateescu CreditAttribution: amateescu commented@dawehner, I don't have any plans, not anymore.
Comment #5
dawehnerDoes that mean that we should better move the route parameter logic just into MenuLink, so it is easier to get it in at the end of the day?
Comment #6
amateescu CreditAttribution: amateescu commentedIf you think that's easier, sure. If you're asking what I'd prefer, it's still fixing this issue :)
Comment #6.0
amateescu CreditAttribution: amateescu commented...not its issue queue.
Comment #7
amateescu CreditAttribution: amateescu commentedThis is the first step needed to fix the performance problems from #1842858: [PP-1] Convert menu links to the new Entity Field API. If we add internal url support for the link field, we would be able to group 5 columns (which are now 5 individual base fields) from the menu_links table into one base field.
Comment #8
anavarreComment #9
anavarreComment #10
anavarreAdded https://drupal.org/project/ckeditor_link which supports internal links as well.
Comment #11
benjy CreditAttribution: benjy commentedAdding beta blocker tag and bumping to critical since this is blocking: #1842858: [PP-1] Convert menu links to the new Entity Field API
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedAre all plans A-D compatible with that goal? What would be each plan's impact to the menu link code, especially with regards to this quote from #2051889-7: Add route parameters property to menu links:
Comment #13
Crell CreditAttribution: Crell commentedAt least in concept, options B or C seem the most viable to me. The unknowns and mismatches around D worry me, and we do need to have internal links in core so that rules out A.
The main question is whether forcing internal and external links to be separate fields poses a sufficient problem to justify the extra work of merging them.
However... if menu entities use a link field, they already support both internal and external links. That means we have a use case in core that demands a single field, because it would be a regression otherwise.
So I think that leaves us with C.
Comment #14
amateescu CreditAttribution: amateescu commentedI also think C is our best / only option if we want to be able to use this field for menu links. I'll start working on this next week, when I come back from vacation :)
Comment #15
amateescu CreditAttribution: amateescu commentedSorry that I forgot to mention (I said it so many times in IRC that I thought I did it here as well), I'm waiting for #2153891: Add a Url value object before moving forward here.
Comment #16
klonos...better status then ;)
...though I guess it won't take long since the other issue is RTBC.
Comment #17
xjmUnblocked. :)
Comment #18
timodwhit CreditAttribution: timodwhit commented+1 to Option C, FWIW. :)
Comment #19
lee20 CreditAttribution: lee20 commented+1 to Option C
Comment #20
rymcveigh+1 to Option C
Comment #21
chrisolof CreditAttribution: chrisolof commentedAdded Link to the list of modules providing this functionality in the contrib space.
Comment #22
amateescu CreditAttribution: amateescu commentedHere's an initial patch that implements option C. No tests yet as I'd prefer to get an approval for this direction and code reviews before writing them.
Comment #23
amateescu CreditAttribution: amateescu commentedAnd the patch :)
Comment #26
amateescu CreditAttribution: amateescu commentedImpressive test coverage!
Comment #27
das-peter CreditAttribution: das-peter commented@amateescu Damn, I was working on that too because I couldn't reach you in IRC :D But I'm glad to see that you've made the same changes I made.
One addition I made was to add support for schema / protocol relative URL's (http://tools.ietf.org/html/rfc3986#section-4.2 / http://url.spec.whatwg.org/#concept-scheme-relative-url). I talked to Crell about that and he agreed that it's a good idea to support that. However, I think I'll create an extra issue for that so I can review this patch.
Comment #28
amateescu CreditAttribution: amateescu commentedUgh, sorry about that, lots of non-Drupal stuff to do these days so I can't stay in IRC all the time. Having support for schema-relative URLs sounds like a useful addition and I agree that we should open a new issue for it.
Comment #29
das-peter CreditAttribution: das-peter commentedShouldn't we return another set of information if it's an external url? Similar to
toString()
/toRenderArray()
?(As far as I've seen this is used in
LinkWidget::massageFormValues()
and I'm not familiar with that)Inline comments must end in full-stops, exclamation marks, or question marks.
Shouldn't we use
\Drupal::formBuilder()->setError()
for new code?That's it, patch looks pretty good to me - I'm tempted to RTBC it but I think someone else should have a look too.
Comment #30
das-peter CreditAttribution: das-peter commentedBtw. #2195983: Support scheme-relative URLs is green by now, reviews appreciated :)
Comment #31
dawehnerI wonder whether we should also take the link_title into account? This is maybe more for a followup.
This seems to be a fragile logic. Should we not store the external property additional?
using an external dependency would have a nice semantic: generating a link requires an url
Can we use $this->t() directly? Note: form_error is also a deprecated function already.
Should we rather just match for MatchingRouteNotFoundException, given that there might be exceptions below you don't want to catch if not needed? Url::createFromPath also tells you what it throws.
Comment #32
amateescu CreditAttribution: amateescu commentedThanks for the reviews!
Re #29:
Re #31:
Drupal/Core/Url
has no knowledge of a 'title' property and I'm not sure we want to add it. But a followup to discuss it is fine :)Comment #35
das-peter CreditAttribution: das-peter commentedAdjusted test to validate with the new return for external URL's of
Url::toArray()
.Comment #36
das-peter CreditAttribution: das-peter commentedComment #37
klonos...remember to hide old patches/files when uploading new ones ;)
Comment #38
amateescu CreditAttribution: amateescu commentedSo the approach seems fine, let's get some nice tests before RTBC.
Comment #39
dawehnerAka. inject the thing.
Comment #40
amateescu CreditAttribution: amateescu commentedYou mean inject the url generator in LinkWidget? Because widgets are not (yet) set up for having things injected, afaik.
Comment #41
tim.plunkettThis needs to throw an exception if it's not external (and have an @expectedException test for it!)
I don't understand these changes at all.
I really feel uneasy about this. The docblock and the assumptions made by calling code are that this is a route (meaning, internal). If we make this change, we'll be forced to either check isset($whatever['route_name']) or $foo->isExternal() every single time...
Now this change I support! No one ever needs to touch this, except for #options, which is common to both.
This could totally be one line :)
Same thing?
Comment #42
amateescu CreditAttribution: amateescu commentedgetRouteName()
/getRouteParameters()
do not throw similar exceptions if the url is external..This code gives me the following results:
Yes, if the calling code doesn't just pass $url to another function/method that accepts it as an argument, it needs to know if the url is external or not. This is the case since we decided to bake in external support to the Url object..
Comment #43
amateescu CreditAttribution: amateescu commentedReroll & ping for reviews/rtbc :)
Comment #45
amateescu CreditAttribution: amateescu commented43: 2054011-link_internal_url_support-43.patch queued for re-testing.
Comment #46
jibranSeems all good to me. Let's see what @tim.plunkett thinks about the patch.
Comment #47
tim.plunkettI still think not using methods internally is a mistake. It will hurt if we ever try to refactor. And some of these changes are just to change it, and completely unrelated to the issue.
Can these really be thrown here? I know MatchingRouteNotFoundException is thrown by createFromPath()
And the second hunk only catches 2 exceptions, not 3.
Comment #48
benjy CreditAttribution: benjy commentedI totally agree with this. Any performance gain we're getting is outweighed by the maintainability of the code IMO. If we decided we wanted to lazy load them properties or do any logic when retrieving the property, accessing them directly causes us a headache refactoring.
Comment #49
amateescu CreditAttribution: amateescu commentedRe: #47
1. So you guys are seriously saying that updating the code of three dead simple methods (toString(), toArray(), toRenderArray()) whenever we feel like doing anything special with those *protected* variables will "hurt" and "cause a headache"? ... I'm speechless.
2. Yes, they can. Fixed the second hunk.
Comment #50
sunI do not think we have a clear standard for when to call internal methods and when it is OK to just use internal properties. I think if the property value is (1) guaranteed to exist (e.g., initialized by the constructor) and (2) is trivial in terms of complexity, then a class instance can certainly make the shortcut to use its internal property directly, instead of calling a method.
But in any case, that's an academical detail which should not hold up this fine patch.
Thanks, @amateescu!
Comment #51
BerdirSorry for the late review, also haven't read anything, if I'm repeating anything that was discussed already, just point me to it.
Setting back to needs work (sorry), as there are a few things that should be addressed/checked. others are just ideas.
Would it be useful to use constants that can be used as bitwise operations, so that LINK_GENERIC & LINK_EXTERNAL/LINK_INTERNAL => TRUE?
The constants could also live on the interface as you added one anyway? static:: would still work. And we could be nice and work against an interface and not a specific class.
The first comment seems a bit stale, as the trim() on url is moved down?
A bit confused that this doesn't seem to handle the route_name case, preSave() is also called then?
If I understood correctly, then this field can either have a url or a route_name + parameter? Should possibly have a !empty() then around the logic here and might even want to keep the trim() at the top as a result of that?
I think those place holders don't provide enough context to make it clear to a translator what they are. Also, "-" in a placeholder is rather uncommon.
Maybe something like %internal_path, %external_path?
That said, I just checked how the block path visibility setting does it, and it's very similar, you probably even copied it from there :)
We check with Gabor, but it's not that important.
The comment here seems wrong, if it supports external and/or internal URL's then it would always be executed. general or internal? Based on my suggestion above about binary constants, this could be $url_type & LinkItem::LINK_INTERNAL. Although those are somewhat uncommon, it might be a bit more self-explaining given you understand it...
Comment #52
sunI like the idea of using bit-masked flags for
LinkItemInterface::INTERNAL
andLinkItemInterface::EXTERNAL
.If we go with that, then we actually ought to be able to remove the ::GENERIC constant, because it can be composed out of the other two, and that is more natural to how bitwise flags are used in PHP core and elsewhere in Drupal.
The annotation is also able to use the constants; i.e., no need to put "2" in there. (Not sure whether the Annotation parser supports bitwise ORs though.)
Lastly, I just spotted a typo that I missed in previous reviews:
Typo in
->route_paramaters
Comment #53
BerdirChanged 1, 3, 4 and updated the comment for 2, not exactly sure how that works.
Also updated the typo in #52, that's a problem with our magic accessors, because they allow anything and don't throw any notices...
Comment #54
amateescu CreditAttribution: amateescu commentedI have to say that I'm not a huge fan of bitmask flags for extremely simple use-cases like this, but I'm not really opposed to it either, so the latest patch looks fine to me. Just a minor thing:
allow :)
Comment #55
YesCT CreditAttribution: YesCT commentedI'm pretty sure the allows/allow was not a core gate, so this probably should not have been moved to needs work for that.
I thought maybe there was something else that it needed work for.
@dawehner thought maybe it was the bit mask thing, but reading back through the comments, looks like there is (enough) agreement that that is fine.
Read through the patch though and found these nits.
And one concern about getPath().
I dont see the "if it is external".
Is that meant to be a warning? Like:
"Use this only if ..."
see:
Maybe it should throw an exception if it's not external?
Hm. seems tim brought this up already in #47.3
tim pointed out in irc that
I'll investigate trying to do something similar next. But I'll post this stuff first.
I was going to say that this looked like it should be $this->isExternal(). Then I read back through the comments and this was already discussed (See @tim.plunkett in #47.1)
Most were fixed in #49, but a couple missed. I did them.
Also, the chunks of code in toArray() and toRenderArray() are identical... but they were identical before this issue anyway.
should not be capital since it's not another sentence, also, I think it should reference the form.
(https://drupal.org/node/1354#functions)
validateTitle and validateUrl are not used anywhere. Oh, wait, maybe they are specified as a callback. Yeah as $element['#element_validate'][].
I looked for examples for OO code, and they were all different.
If there is an issue to decide on a way to document OO form validation methods, we should fix this in that issue.
I'm leaving these alone.
Comment #56
YesCT CreditAttribution: YesCT commentedBefore that:
cd core;
./vendor/bin/phpunit --group Url --strict
OK (20 tests, 65 assertions)
after that:
same.
adding the exception did not make any tests fail.
means missing test coverage.
there is no getPath() called from any test.
might look to add something to ExternalUrlTest
[edit:}
only call to getInternalPath() in an assert is to make sure it returns null (for an external path). So that is probably missing test coverage also (that it returns something not null when it should). But that might not be the fault of this issue.
Comment #57
YesCT CreditAttribution: YesCT commentedadded tests (not the "fix" throwing the exception)
and got:
$ ./vendor/bin/phpunit --group Url --strict
PHPUnit 3.7.21 by Sebastian Bergmann.
Configuration read from /Users/ctheys/foo/drupal/core/phpunit.xml.dist
................F.....
Time: 2 seconds, Memory: 47.50Mb
There was 1 failure:
1) Drupal\Tests\Core\UrlTest::testGetPathForInternalUrl
Failed asserting that exception of type "\Exception" is thrown.
FAILURES!
Tests: 22, Assertions: 68, Failures: 1.
With the fix:
OK (22 tests, 69 assertions)
Head (no patch at all) is:
OK (20 tests, 65 assertions)
I need feedback because I'm still new at phpunit tests and ran out of time to look @depends.
and
oh, this was kinda cheating I think... :) since I'm just checking it wasn't null.
I wont be back at this for several hours, so anyone can take this on.
Comment #58
dawehnerRegarding the named exceptions: I think \UnexpectedValueException could explain things semantically a bit better.
Comment #59
amateescu CreditAttribution: amateescu commentedSorry, the 'needs work' from #54 happened because I had a very old browser session where the status field was set to NW.
Here's a review for the latest changes:
If we want to do this in this patch, we need to do it for getRouteName() and getRouteParameters() too. (throw an exception if the url is external)
Comment #60
effulgentsia CreditAttribution: effulgentsia commentedReroll.
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedThis patch looks awesome! Just some minor nits. Also, the issue summary could use an update to just explain the plan chosen and any relevant details about implementation, plus screenshot(s) of any UI screens that changed. And, if we're doing D8->D8 change records, then this needs one, I think.
We also store route name and params.
Would LINK_EXTERNAL make a better default to match HEAD?
Why?
We'll need a migration issue for this, so tagging as "needs migration" just to make sure that issue gets filed.
I think we always capitalize "URL" in screen text, or has that changed at some point?
Why check LINK_INTERNAL here? We already have that logic in the code that assigns this validation function. And if someone really wants to run this function on a external-only link (e.g., they want to use a #type='textfield' instead of #type='url' there for some strange reason), why not let them?
Comment #62
amateescu CreditAttribution: amateescu commentedThanks :)
Also addressed my own review from #59 and I'll update the issue summary a bit later today.
Comment #63
BerdirDoes the comment here need an update?
Issue summary and screenshots still needed :)
Comment #64
amateescu CreditAttribution: amateescu commentedRerolled, fixed the comment from #63, updated the issue summary and here's a screenshot with the new instance setting.
Comment #65
BerdirThanks, this looks good to me, 1+ to RTBC.
This is tagged as Needs change record, but I'm not sure what change we need to record exactly? This doesn't seem big enough to require a 8.x -> 8.x change record. Maybe update the Url change record a bit, but we apparently don't have one for that yet? At least I wasn't able to find one.
Comment #66
dawehnerThis looks really great!
The variable name is a bit odd, but it was like that before.
Maybe this is a bad question but can't we extract this logic somewhere? Maybe to a constraint, not sure whether this makes sense at all, but ths logic here does not seems to be bound to a widget.
Comment #67
tim.plunketthttps://drupal.org/node/2189619 is a draft change notice from #2153891: Add a Url value object, no one ever reviewed it I guess.
Comment #68
amateescu CreditAttribution: amateescu commentedI agree that the validation logic should be extracted to a constraint. Unfortunately, due to some personal stuff I will be offline for about a week, so anyone can feel free to take over this issue :)
Comment #69
blueminds CreditAttribution: blueminds commentedI came across one problem here. If you use the link field via UI all is fine. But doing an update of an entity having this field programmatically will result in loss of the query data. That is due to the logic in the preSave() method, where it expects this->url to also have the query appended, which is not the case if you programmatically any other property but the url.
Basically the options value that is reset in the preSave() should be reset only in case the url value was intentionally reset. How we can tell this, not sure.
Comment #70
blueminds CreditAttribution: blueminds commentedDiscussed this with Berdir and he suggested to move the logic from preSave() into the widget's massageFormValues(). Please see attached patch.
Comment #71
JayeshSolanki CreditAttribution: JayeshSolanki commentedComment #72
JayeshSolanki CreditAttribution: JayeshSolanki commentedComment #73
pwolanin CreditAttribution: pwolanin commentedSo, I think this needs 2 changes.
We shouldn't be forcing with widget code to do bitwise logic - that would better be exposed via helper methods to flag it as allowing internal or not.
Also, per #66/#68 let's move validation out of the widget form.
I'll work on these items.
Comment #74
BerdirNote that for methods to work you either need to call $entity->field[0]->someMethod() or you need to provide a list class for it in the field type annotation.
Comment #75
pwolanin CreditAttribution: pwolanin commentedIn the patch code I seem them calling $this, like $this->isExternal()
Comment #76
pwolanin CreditAttribution: pwolanin commentedThis cleans up terminology to refer to link type, not url type. Make the render array method actually return a valida render array. Also fixes doxygen, makes exception types consistent, and removes an extra function call slowing down link rendering.
Comment #77
BerdirNot sure how exactly we need to document them, but never seen it done like this :)
Comment #79
dawehnerReally not sure whether we should do it, as url has not really something in common with links, in which case we should better put a static method on some other helper
We probably have to add an @return
Comment #80
pwolanin CreditAttribution: pwolanin commenteddiscussed in person with dawehner.
Drupal\Core\Form\ConfirmFormInterface::getCancelRoute() is not documented as returning a Url object, so if we want to make that change, it should be a follow-up patch to change the interface and all implementations.
removed toLinkRenderArray()
fixed doxygen.
Comment #81
dawehnerWe should extend test coverage and maybe also fix the ones which fail: ConfirmFormHelperTest
Comment #82
pwolanin CreditAttribution: pwolanin commentedtest fixes
Comment #83
pwolanin CreditAttribution: pwolanin commentedbetter method name and comment in the test.
Comment #87
pwolanin CreditAttribution: pwolanin commentedWe should remove the alias manager dependency from the link generator, since it's not needed.
Comment #89
tim.plunkettPlease don't change this?!
If the docs on getCancelRoute are wrong, we should fix then. That absolutely returns a Url object in places.
I haven't looked at any of the other changes since #70...
Comment #90
tim.plunkettIf we have no coverage for getCancelRoute returning a URL that's my fault, but it needs to be supported.
See #2215961: Entity::urlInfo() should return \Drupal\Core\Url for a patch that depends on this.
Comment #91
amateescu CreditAttribution: amateescu commentedI don't see the reason for all the changes since #70. Everyone said the patch is fine and it only needed the validation code to be moved to a constraint, but every update since then did anything but that..
Comment #92
pwolanin CreditAttribution: pwolanin commented@tim.plunkett - I can put it back, but then I think any patch that's actually adding Url objects as return values should convert them all and the interface - it should pretty mechanical.
Right, yes, constraint. Got distracted, obviously.
Comment #93
pwolanin CreditAttribution: pwolanin commentedHere's a first attempt to move to using constraints.
The docs on constraints are... lacking. And there are some field API bugs like #2226723: Value changes from WidgetBase::massageFormValues() are discarded
Adds back basic support for Url in confirm form helper, though I still think it should be out until it's the sole value accepted.
Initially I implemented the constant as a method:
But this patch now has it declared via annotation.
Comment #94
tim.plunkettI still don't understand why this file is changed at all. Other than code flow, what have you changed here?
Where did this go?
Comment #95
pwolanin CreditAttribution: pwolanin commented@tim.plunkett. The toRenderArray() method didn't return an actual render array: it returned a fragment that couldn't be rendered by drupal_render().
Should the Url class know about link type render elements? In discussion with dawehner, it seemed clearer to move tis responsibility to link-related code. In an earler iteration I had it return a full link element, but I think removing it is the more sensible option.
Given that the toRenderArray() method is removed, ConfirmFormHelper needs to be adjusted.
Comment #96
tim.plunkettPlease revert both of those changes. I have no idea what you discussed, as you didn't post any of it here or open any other issues.
As I stated before, #2215961: Entity::urlInfo() should return \Drupal\Core\Url relies on both of those remaining as-is.
Comment #98
tim.plunkettOpened #2226871: ConfirmFormInterface::getCancelRoute() should always return Drupal\Core\Url.
Comment #99
pwolanin CreditAttribution: pwolanin commented@tim.plunkett - regarding the render array method, it was returning an incomplete fragment, and really it isn't clear whether a Url should support this at all.
I initially suggested making it return a link element in a patch above, but it seemed more coherent to simply remove it after discussing with dawehner and now fago. We can put it back as a link element if it's really important for some reason, but returning a non-renderable fragment doesn't make sense.
Changing the Constraint and Validator to be in one class as a preview of the pattern change in #2226821: Combine Drupal Constraint and ConstraintValidation classes into one
I feel like there are some unresolved UX questions - like whether user's can change the field setting after it's initially set? What does that mean if e.g. you change is so existing data becomes invalid? In discussion with fago, we should do an EFQ to find out what data exists before allowing the user to change the setting.
Comment #100
pwolanin CreditAttribution: pwolanin commentedhad to add
'link_type' => LinkItemInterface::LINK_GENERIC,
to the test code to avoid fails.Apparently the default configuration is not getting picked up from the annotation. dawehner will try to figure out why.
This replaces the interface constant with a number, but it's still not working. Ideally better to use the constants.
Comment #101
dawehnerThis should be it.
Comment #103
tim.plunkettOnce again. Please move all of the ConfirmForm changes to a separate issue, and the toRenderArray removal to another issue as well.
None of those changes are in scope, and weren't in any patches before @pwolanin hijacked the issue in #76.
We need to discuss those changes, and that shouldn't hold up this issue.
Comment #104
sunI didn't really study recent patches, but solely based on following recent comments via e-mail, I tend to agree with @tim.plunkett — This patch was RTBC already, and out of a sudden we started to revamp the originally proposed and working code + seemingly much more code.
Can we go back to the patch that was RTBC, give or take some later improvements, and move forward?
Comment #105
dawehnerComment #107
tim.plunkettThanks for removing the ConfirmForm stuff. But we do need those isExternal parts of toArray and toRenderArray.
Comment #109
tim.plunkett107: internal_url-2054011-107.patch queued for re-testing.
Comment #110
tim.plunkettRandom failure.
Comment #111
jibranCan we change it so that it'll be within in 80 char limit.
$value is not set to null so it should be set and we should typehint it to LinkItem in function definition.
more then 80 chars.
:/
Comment #112
dawehnerRebased.
This would conflict with the parent interface.
Comment #113
dawehnerSome other small cleanup.
Comment #114
jibranReturns
Comment #115
tim.plunkettNope, those are not the method docs, so they're not subject to those rules.
I barely touched this code, only undoing an overzealous change. So I'm going to RTBC this.
Comment #116
dawehnerJust changed it.
Comment #117
xjmUsually we don't put any verb in the return description. :P So it's silly to worry about either way.
Comment #119
Dries CreditAttribution: Dries commentedCommitted to 8.x! Let's make progress on that beta blocker now. Thanks to the many contributors on this issue.
Comment #120
moshe weitzman CreditAttribution: moshe weitzman commentedAs a followup,
Comment #121
blueminds CreditAttribution: blueminds commentedAdded a followup: #2231413: Link field does not accept a valid path alias
Comment #122
jessebeach CreditAttribution: jessebeach commentedThis was committed without a change notice :)
Here's the draft.
https://drupal.org/node/2231761
Comment #123
jessebeach CreditAttribution: jessebeach commentedComment #124
jessebeach CreditAttribution: jessebeach commented'Needs change record' tag to 'Missing change record' tag.
Comment #125
dawehnerHA, that change notice could be written by some members of the VDC team directly :p
Note: https://drupal.org/node/2189619 has a lot of those information.
Comment #126
effulgentsia CreditAttribution: effulgentsia commentedWhat are the kinds of things we need for D8 to D8 change record? For example, do we need a change record for the link field's 'attributes' property being changed to 'options'? Or anything else that a contrib module implementing a formatter or widget for link fields might need to care about?
Comment #127
xjmD8-D8 change records are only for major changes that significantly impact core and contrib developers, not every little API change. The more important (and time-consuming) task for issues like this is usually reviewing existing 7-8 change records and making sure they reflect/include the new change... the design of the change record system didn't quite account for the scope or length of this release cycle. ;)
Comment #128
amateescu CreditAttribution: amateescu commentedReseting to the proper status then.
Comment #129
xjmhttps://drupal.org/node/2231761 is completely empty; is nothing needed at all?
Comment #130
effulgentsia CreditAttribution: effulgentsia commentedCorrect. The Link module is new to core for D8. I added the "needs change record" tag in #61 in case we wanted a change record letting people know that it can now support internal paths, or that 'attributes' was changed to 'options', which would affect contrib modules that interact with link fields. But per #127, I don't know that either of that qualifies as major enough to warrant a 8->8 record.
Comment #131
xjmAlright, I'll remove the draft. Thanks!
Comment #133
Berdir