Problem
Goal
- (Heavily) Clean up and simplify Link module and move it into core.
Clean-up notes
Link module's code was pretty clean under @quicksketch's command. However, maintainers seem to have switched (multiple times?) recently, introducing some not-so-clean code, and the module was never really upgraded/refactored for D7:
http://drupalcode.org/project/link.git/tree/refs/heads/7.x-1.x
- Remove support code for Migrate and Devel.
- Remove support code for Views (might be re-introduced later).
- Massively clean up and refactor the code to follow coding patterns of field type modules in core.
- Improve the UI/UX. (mainly administrative)
- TBD: Remove advanced features.
Original summary:
Along with text, numbers, dates and images, URLs are one of the basic data types on the web. The link module does most of what we need (maybe even a little too much). Some of its helper functions for parsing and completing URL's could be consolidated into core include files so that other modules (filter.module?) could also use them.
Comment | File | Size | Author |
---|---|---|---|
#55 | platform.link_.55.patch | 34.09 KB | sun |
#55 | interdiff.txt | 9.42 KB | sun |
#40 | platform.link_.40.patch | 29.04 KB | sun |
#40 | url-link-interdiff.txt | 17.69 KB | sun |
Comments
Comment #1
bjaspan CreditAttribution: bjaspan commentedIMHO, core does not and should not contain everything a site might need, even "all of the basics." We have contrib for a reason. I would vote against the URL field.
However, I know that Dries and I disagree on the fundamental concept of what should be in core and why.
Comment #2
rickvug CreditAttribution: rickvug commentedI agree that the "move X to core" train has to stop at some point. However, I would disagree that field types such as Date, URL, Filefield or Imagefield do not belong in core. These field types and would be used on the majority of sites and are flexible building blocks. Shipping with core has a number of advantages:
- We can leverage the fields in core install profiles.
- Better initial experience for end users. They would show off the Fields UI (if that gets in).
- These fields are needed for a proper upgrade path for profile module.
- Parts of the field modules could be abstracted, placed into inc files and re-used. For example, the Date API or url parsing functions.
Note: cross posting at #501428: Date and time field type in core since the conversation is the same thus far
Comment #3
yched CreditAttribution: yched commentedI think we can now say it won't happen in D7.
Comment #4
geek-merlin:-)
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #6
klonos...coming from #501428: Date and time field type in core
Comment #7
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedsubsubsub
Comment #8
RobLoachhttp://drupal.org/project/link ... Would be nice to have a URL Form API field type though.
Comment #9
yched CreditAttribution: yched commented@Rob Loach : agreed, would be more inline with how things were done in D7 with formatted text, file upload... - provide powerful FAPI types, and let field widgets use them.
Comment #10
bangalos CreditAttribution: bangalos commentedAdding "nofollow" to URLs is being discussed at #102468: Add rel="nofollow" to profile url fields
Comment #11
sunBetter title. Tagging. See #1668292: Move simplified Profile module into core and related Profile in core issues.
I've kick-started actual work on this in the platform-link-sun branch of the Platform sandbox. Any help would be highly appreciated! See Platform sandbox page for basic instructions.
Comment #12
sunI've also created #1668360: Core developer git access for 8.x-1.x for the Link project, as it might be better to work directly on the 8.x-1.x branch of the contrib module first. That way, the initial/basic clean-ups and improvements might be backportable to D7.
In any case, I do not plan to hold off this effort very long on organizational/workflow questions. Instead, my goal is to have a concrete result in the next couple of days.
Comment #13
geerlingguy CreditAttribution: geerlingguy commentedWhile smallcore and its ideas are important to keep in mind, I know that having the link module (a basic URL field) in core would save me from having to keep the module installed/up-to-date on 90% of the Drupal sites I've built. Almost any site with a profile uses at least one URL field, and most other sites also have use for URL fields as well.
I'm +1 for this being in core (after a major refactor/cleanup of course—the module's seen a couple of rewrites between D6 and D7, and it's still a bit disorganized and bloated, imo).
Comment #14
RobLoach"url" HTML5 form types are in Drupal 8 now too. So now we might just need the Field API support.
Comment #15
sunYes, that's of course what I had in mind.
Meanwhile, @jcfiala, the current primary maintainer of Link module, mentioned that he'll grant git access to Link's 8.x-1.x branch, so we can effectively prepare the entire module in the existing contrib repo for core inclusion. This also means that, in the unlikely case we will not move it into core, there would be a ported and readily available Link module for D8. Overall, much better procedure. :)
As soon as access has been granted, I'll redo my changes in Link's repo.
Comment #16
sunI need someone to help with the overall module clean-up. (see #1668360: Core developer git access for 8.x-1.x)
Comment #17
sunActual work on porting and cleaning up Link module has started in Link module's repository, with some good progress already.
However, #1674290: PSR-0 tests in contrib modules are not found kinda causes some troubles for this approach currently... I resorted to manual testing + occasional manual/local test runs for now, but we should fix that rather sooner than later.
Comment #18
amateescu CreditAttribution: amateescu commentedFWIW, this is the workaround that I came up with during the Views sprint in Barcelona: http://drupalcode.org/project/views.git/commitdiff/8d37c7e64222bbda1fa6d...
Comment #20
sunyeah, I've seen that one. Worked on a patch for PIFR instead: #1674290: PSR-0 tests in contrib modules are not found ;-)
Comment #21
Dave ReidI'd like to offer a simple alternative to Link module in core. I would much rather just like to see a simple URL field type that only has one column: 'url' stored as SQL text type. And two formatters: plain text and linked URL (default). I would argue that the current Link module is more a complex field and might be better left for contrib for D8. This gives us what we need for migrating Profile module data since it only stores just the URL.
Comment #22
Dave ReidComment #23
klonosDo you think that offering a way to provide an optional title would make it too complicated for core?
Comment #24
sunAs briefly discussed in Munich, I think an additional title per link/url really makes sense. At least I extensively used the title option in past projects, whereas especially the user-defined/dynamic/static link text/title options are the primary enabler for making the Link field suitable for many different use-cases.
There are some additional options in Link module that probably don't make sense for core, but in general, I'm comparing the situation to the File/Image fields in core, which apparently have quite a lot of options, too, so the question of where to draw the line is a bit fuzzy, but we can figure that out later, and I'm super happy to do so.
We're cleaning up and preparing the Link module for D8 in its contrib project. The result of that will be a core patch, and based on that, we can decide which additional features we can drop for core.
The additional benefit of doing the work in the existing/actual contrib project is that there will be a fully ported D8 version of Link module at the time when D8 is released, in case we won't be able to make this happen.
Comment #25
Dave ReidSo I whipped up a much smaller 'URL' field type for Drupal 7 and 8 with http://drupal.org/sandbox/davereid/1762346 which can easily be provided as a patch to include the module in core/modules/field/modules. I would encourage people to take a look and I will likely be rolling this as a core patch this weekend.
Comment #26
sunUm. There are quite a couple of things I don't like about that:
1) Restricting to external URLs is an extreme limitation that makes the field unsuitable for many use-cases.
2) Removing the ability to enter a custom label/title per URL/link makes the field unsuitable for even more use-cases.
3) Auto-fetching the HTML page title from the remote resource through an HTTP request is something for contrib.
4) I seriously don't get why we're trying to be smarter than an existing solution that has sufficiently proven itself for a long time in contrib already. Yes, it needs to be cleaned up. Yes, some advanced options of it might be too much for core and we can remove them. But that's not really a reason to re-invent the wheel — even more so, a wheel that is incapable of solving use-cases of which we know to exist.
Please have a look at the 8.x-1.x branch of Link module, in which we're actively cleaning up the module to make it suitable for core:
http://drupalcode.org/project/link.git/tree/refs/heads/8.x-1.x
Comment #27
Dave ReidUnfortunately using '#type' => 'url' only supports external links. This is the basis of the module.
The user still has the ability to edit titles for each link.
I personally did not like what I saw so far in the 8.x-1.x branch of link and just want to have a simple version.
Comment #28
Dave ReidSupporting internal links is as easy as writing a contrib field widget.
Comment #29
sunAs mentioned, the clean-up of Link module is in early stages, so I can perfectly see how there is lots of code that is not to like. We've already removed and cleaned up quite some code, but the effort is totally not done yet. It really doesn't make sense to duplicate efforts here :(
In the end, the goal is to move a link/url field into core that is simple but yet suitable for more than a single use-case.
Comment #30
klonosI'd like to see D8 support link fields with the ability to link to ANY URL (be it internal/external) too. I find
target
andrel
very useful for SEO and when I want users to stay on the original site. I don't think these as edge cases at all.Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedFWIW, Dries has expressed a desire for Link and Date Fields in core. Hopefully folks can keep working on this.
Comment #32
Dave Reid@moshe Feel free to take a look at http://drupal.org/sandbox/davereid/1762346 and evaluate it since I'll be rolling a patch for it soon.
I consider URL target attribute data better suited for a more advanced field formatter.
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commentedThat lightweight alternative looks great to me. Thanks Dave.
Comment #34
sunHrm. Would it be possible to set up a skype call to discuss this? This is too important to have a needless fight over different approaches/code-bases. And we all certainly have better things to do than to work on duplicate and potentially obsolete stuff in the coming weeks. The lightweight URL module is too lightweight for me. I understand the concerns against the current Link module code. I'd like to talk to find a compromise and single target for joined development efforts.
Comment #35
andypostAnother module and question in #1778858: Decide on built-in support for internal URLs
Comment #36
Dave ReidI'm open to a call, but this week would be tough for me to schedule time. I've got a project nearing completion that I need to be heads down on, and lots of baby stuff to handle on my off-time as well.
Comment #37
Danny EnglanderIn terms of #29 above (@sun), item #3:
I really use those the different URL options quite often when I am building out / theming a site. The most important for me are:
It would be great if these can be left intact. Thank you.
Comment #38
sunErr. I just had a look at the URL module's queue at http://drupal.org/project/issues/url, and I'm sorry, but that module is turning into a 100% duplicate of Link module. Re-inventing the wheel for absolutely no reason. :(
That makes no sense to me. I'll keep on cleaning up and polishing Link module and post a first patch here soon.
Comment #39
amateescu CreditAttribution: amateescu commentedThe url widget can also be used for the path property of menu links in #916388: Convert menu links into entities so I'd +1 the cleaned-up Link module because it already has support for internal URLs.
Comment #40
sunHere we go: Link Reloaded™ :)
Spent the better half of this weekend to get the 8.x-1.x branch of Link module cleaned up, simplified, up to par with current Drupal core, and tested.
It's really simple.
And as some of you might notice, I obviously did not re-invent the wheel for quite some code — instead, I borrowed and incorporated the better parts and ideas of URL module. In fact, the code of the modules is so similar that I'm going to attach a small interdiff between url.module and link.module. I hope this helps to make my earlier point of unnecessary duplicated efforts clear... ;)
That said, this inherently means that at least @Dave Reid should also be credited when this is committed. :)
Also, as already mentioned in #29, the revamped code uses #type 'url' for the link field widget. This means that only external links are supported. Support for internal links is not a hard requirement for getting the new field type into core. However, there's a great discussion going on for the URL module already, and I'd recommend to move that issue into the core queue later, so we can investigate whether we can natively support internal URLs for the Link field.
I agree with @highrockmedia's analysis on useful/required formatters in #37 — those are the only Link formatter options that remained. However, all but one of the options have been converted into formatter settings of a single default "Link" formatter. Only the "Separate title and URL" formatter is a separate formatter.
Although I think this code is pretty much ready, I performed a fully-fledged git-subtree-join of the contrib repository branch into the platform-link-501434-sun branch of the Platform sandbox, so as to simplify any possibly required follow-up tweaks to the code (which can thus happen in either of both repos). I'll remove the
.gittrees
file from this patch once this is RTBC or close to RTBC.From my own perspective, the only part that does not look 100% optimal is the second "Separate title and URL" field formatter. It uses a theme function to format the output. Given my own history/experience with link field use-cases on production sites, I still think that this formatter is in the ~80% range of overall use-cases and should thus be built-in. That said, I think it's only "suboptimal", but not really a blocker — we can still improve this code post-commit.
What do you think?
Comment #42
sunStrange random testbot failures, or possibly rather testing infrastructure failures elsewhere (not LocaleImportTest this time)... manually re-tested, it's green now.
Comment #43
chx CreditAttribution: chx commentedI am very glad some clean and well tested code has materialized. Maybe we can move ahead with just this one but I am really sad not to see any internal support. With all the CMI work ongoing to support a dev-prod scenario, I think we really need to consider internal support as in the linked discussion . Of course, we can decide it's a folllowup. I am unsure.
Comment #44
chx CreditAttribution: chx commented<rant>
Also totally off topic but this issue totally shows why I dislike sandboxes. There's no visible history in this issue for what considerations went into the patch, what refactors happened, what problems arisen and were solved. Yes, I could look into the sandbox but a commit message will never have the sort of commentary an issue followup would have. Sandboxes are surely useful for several people working together -- but even so I would love to see some progress report from time to time :/
Oh well.
</rant>
Comment #45
sunThat's why this effort did not happen in a sandbox to begin with. See Link 8.x-1.x.
That said, I admit that I personally expected some more involvement from others, especially given that the porting effort happened in the official Link project, publicly visible for everyone, but alas, the idea of D8 itself and on its own might still be a huge barrier for many. ;)
Regarding support for internal links, I'd recommend to make that a follow-up issue/discussion. All of the options being investigated in the existing discussion are "tacked-on" and thus do not really block the initial patch. Nothing stops us from improving status quo. It's just that the current status quo is nothing. ;) The quicker we get this in, the faster we can advance and improve on it. Also, moving that discussion into the core queue will definitely lead to way more traction.
Comment #46
DamienMcKennaIMHO given there are two mostly-identical modules (Link, URL) for the same basic task (linking to things) I think further development should wait until the two developers in question (Sun, Dave Reid) get to discuss their modules. We gain *zero* benefit from having a duplicate here, all effort should focus on ensuring that one option a) works well and b) is clean & stable. So, hows about it gents?
Comment #47
sunHappy to clarify that:
There's only one tidbit and one feature that have been left out from URL module:
- URL module's field widget supports a setting to control the
#size
of the link form input field. I'm not aware of a similar setting/option in a field module in core that allows to configure the size/length of the form element, so I left that out.- URL module has an advanced feature baked-in, which automatically fetches the page title of a given URL (note: user input) by performing an outbound HTTP request against the given endpoint, and assigns that as the link's title on success. I think it's safe to say that this feature is not core material. The current code in URL module consumes only ~10 lines, but a core-worthy implementation would have to be much more advanced, and even that single proposal of its own would certainly trigger a 200+ comment thread of privacy and DoS concerns... ;)
The lack of actual architectural differences is why I additionally provided a diff between url.module and link.module in #40, which shows that the functionality and implementation is essentially the same.
There's one additional issue though — neither of both modules handles the compound field widget that may consist of two form elements really well. I already looked throughout core, but we don't seem to have a clear pattern and ideal/prime implementation for that case yet. My hope is that we can explore and improve the Link field widget UI/UX in a separate follow-up issue.
Comment #48
Lars Toomre CreditAttribution: Lars Toomre commentedI for one world like to hear @Dave Reid's thoughts here before this proceeds any further. It sounds like that there is not agreement from both key competing module authors.
Comment #49
DamienMcKenna@sun: Apologies, I hadn't realized the extend to the work you did in #40 above - good stuff!
On a related note, what are your thoughts on an upgrade process from D7 given the feature changes? Are you going to do a 7.x-2.x branch that is identical feature-wise to the D8 version?
Comment #50
moshe weitzman CreditAttribution: moshe weitzman commentedNice work!
IMO, most internal links are best handled as entity references, and even more so in D8 since entities have become much more flexible/powerful. That leaves just internal links to non-entities, and thats pretty edge case. So, I agree with deferring those to a follow-up.
Will try to review this soon.
Comment #51
sunre: #49 @DamienMcKenna:
The discussion on E-mail field for core already concluded that Drupal core does not provide an upgrade path from contrib modules; thus, that will have to be done and provided by contrib. Regarding D7, Field API didn't really diverge (yet), so backporting the 8.x-1.x code into 7.x-2.x should be pretty straightforward and doable in no time. I already created a backport issue along those lines, which could be re-purposed. In general, that might be a very good idea to do, as it means that contrib can deal with the 7.x-1.x to 7.x-2.x upgrade path now, and can also figure out what to do about the features that have been removed. However, let's move that discussion into aforementioned issue, because it's OT here. ;)
Comment #52
sun#40: platform.link_.40.patch queued for re-testing.
Comment #53
sunAny chance to move forward here?
Comment #54
rbayliss CreditAttribution: rbayliss commentedThis looks awesome! One bit of weirdness I noticed is that if you enter a link with a query string and/or fragment, the separate title/url formatter cuts off the query string and fragment when it displays the link, whereas the other formatter doesn't. Otherwise, it looks ready to roll.
Comment #55
sunThanks for testing! I've fixed the separate formatter and added tests.
Added tests for link query and fragment output to testFormatter().
Added tests for 'link_separate' formatter.
Merge commit '0058c8ae3b6e61f41fd4f407b50c28a5e51688ef' into platform-link-501434-sun
Removed .gittrees.
Comment #56
sun#55: platform.link_.55.patch queued for re-testing.
Comment #57
ParisLiakos CreditAttribution: ParisLiakos commentedI am impressed, this is awesome:)
I applied the patch, played around with the formatters everything works flawlessly.
I am gonna be hasty and rtbc this, so we might have some time potentially adding internal url support in follow up issues..
Comment #58
webchickThis looks like a great little patch, but it will need a re-roll for #1785256: Widgets as Plugins.
Comment #59
sunCan we do that in a follow-up issue, please? We purposively added a temporary BC layer there.
Comment #60
webchickOh, sorry. I forgot about the BC layer for widgets. Sure.
I didn't get a chance to look through Dave Reid's stuff (and looks like he's not been able to get back to this in a couple of weeks), but from #40 it looks like a good attempt was made to reconcile the two approaches, and reading the tests here this looks very comprehensive. I tend to agree that the best way to handle internal links might be tricky and should probably be a follow-up (sun, interested in getting entity reference into core as well? ;))
Committed and pushed to 8.x (with Dave Reid credited as well), with a CHANGELOG.txt entry. :) Yay.
Comment #61
swentel CreditAttribution: swentel commentedAdded a follow up for the conversion to plugins - #1796316: Convert Link/URL widgets / formatters to plugin system - it's still in our sandbox, but I'll be moving all of them soon to the core queue!
Comment #62
catchI think we need to add a change notice here for the new module.
Comment #63
swentel CreditAttribution: swentel commentedHere's start, should be enough I guess, also added the e-mail field type: http://drupal.org/node/1796546
Comment #64
catchThanks!
Comment #65
amateescu CreditAttribution: amateescu commentedComment #66
klonosHas anyone filed a follow-up for internal URL support? If so, then please link. If not, let me know and I'll file one. Thanx.
Comment #67
sunI followed up on the existing dicussion in #1778858: Decide on built-in support for internal URLs and asked whether the issue can be moved into the core queue, or whether we need to duplicate it. No response yet.
Comment #68
klonosThanx Daniel. I was already following that issue, but I was looking for one filed against core.
Comment #70
sunFinally came around to create the follow-up issue for this:
#1829202: Make #type 'item' work outside of a form context to render a compound label + content
Comment #71
falcon03 CreditAttribution: falcon03 commentedJust for reference, these are two followup issues about the link field accessibility that we need to solve before releasing D8:
#1874354: Rename current "link title" and add support for the true link title attribute
and
#1801268: Change link field 'title' field to 'link text'
Help would be greatly appreciated :-)
Comment #72
Dave Reid#1796316: Convert Link/URL widgets / formatters to plugin system is a followup as well.
Comment #73
yched CreditAttribution: yched commentedFYI : #2076321: Link "title" validation should use a static method
Comment #73.0
yched CreditAttribution: yched commentedUpdated issue summary.