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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bjaspan’s picture

IMHO, 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.

rickvug’s picture

I 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

yched’s picture

Status: Active » Closed (won't fix)

I think we can now say it won't happen in D7.

geek-merlin’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (won't fix) » Active

:-)

Damien Tournoud’s picture

Category: task » feature
Issue tags: -Fields in Core
klonos’s picture

Sylvain Lecoy’s picture

subsubsub

RobLoach’s picture

Issue tags: +link

http://drupal.org/project/link ... Would be nice to have a URL Form API field type though.

yched’s picture

@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.

bangalos’s picture

Adding "nofollow" to URLs is being discussed at #102468: Add rel="nofollow" to profile url fields

sun’s picture

Title: URL field type in core » Move Link/URL field type into core
Assigned: Unassigned » sun
Priority: Normal » Major
Issue tags: -link +Platform Initiative, +Profile in core

Better 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.

sun’s picture

I'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.

geerlingguy’s picture

While 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).

RobLoach’s picture

"url" HTML5 form types are in Drupal 8 now too. So now we might just need the Field API support.

sun’s picture

Yes, 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.

sun’s picture

I need someone to help with the overall module clean-up. (see #1668360: Core developer git access for 8.x-1.x)

sun’s picture

Actual 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.

amateescu’s picture

FWIW, this is the workaround that I came up with during the Views sprint in Barcelona: http://drupalcode.org/project/views.git/commitdiff/8d37c7e64222bbda1fa6d...

sun’s picture

yeah, I've seen that one. Worked on a patch for PIFR instead: #1674290: PSR-0 tests in contrib modules are not found ;-)

Dave Reid’s picture

I'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.

Dave Reid’s picture

Issue tags: +html5
klonos’s picture

Do you think that offering a way to provide an optional title would make it too complicated for core?

sun’s picture

As 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.

Dave Reid’s picture

So 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.

sun’s picture

Um. 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

Dave Reid’s picture

Unfortunately 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.

Dave Reid’s picture

Supporting internal links is as easy as writing a contrib field widget.

sun’s picture

As 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 :(

  1. Oh, hm, I forgot about that detail of #type 'url'. Definitely planned and started to use that for Link, so we might have to punt on support for relative/internal links.
  2. The Link field options I care most about are the optional/required title and the ability to set a static title.
  3. The various options for the URL value are pretty much unnecessary, except for the ability to leave the URL empty while specifying a title. A very common and typical use-case for that is when an author prepares a content that is supposed to link to a resource, which may not exist yet - so the content can be fully prepared already, just needs the final link to be inserted. A content management system should allow for that.
  4. The options for specifying 'target' and 'rel' attributes look pretty important to me as well.
  5. The ability to cut off URLs on display after a certain length seems pretty essential to me, too.

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.

klonos’s picture

I'd like to see D8 support link fields with the ability to link to ANY URL (be it internal/external) too. I find target and rel 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.

moshe weitzman’s picture

FWIW, Dries has expressed a desire for Link and Date Fields in core. Hopefully folks can keep working on this.

Dave Reid’s picture

@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.

moshe weitzman’s picture

That lightweight alternative looks great to me. Thanks Dave.

sun’s picture

Hrm. 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.

andypost’s picture

Dave Reid’s picture

I'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.

Danny Englander’s picture

In terms of #29 above (@sun), item #3:

The various options for the URL value are pretty much unnecessary...

I really use those the different URL options quite often when I am building out / theming a site. The most important for me are:

  • Title, as link (default)
  • URL, as plain text
  • Separate title and URL
  • URL, as link

It would be great if these can be left intact. Thank you.

sun’s picture

Err. 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.

amateescu’s picture

The 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.

sun’s picture

Status: Active » Needs review
FileSize
17.69 KB
29.04 KB

Here 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?

Status: Needs review » Needs work

The last submitted patch, platform.link_.40.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Strange random testbot failures, or possibly rather testing infrastructure failures elsewhere (not LocaleImportTest this time)... manually re-tested, it's green now.

chx’s picture

I 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.

chx’s picture

<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>

sun’s picture

That'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.

DamienMcKenna’s picture

IMHO 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?

sun’s picture

Happy 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.

Lars Toomre’s picture

I 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.

DamienMcKenna’s picture

@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?

moshe weitzman’s picture

Nice 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.

sun’s picture

re: #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. ;)

sun’s picture

#40: platform.link_.40.patch queued for re-testing.

sun’s picture

Any chance to move forward here?

rbayliss’s picture

This 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.

sun’s picture

FileSize
9.42 KB
34.09 KB

Thanks 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.

sun’s picture

#55: platform.link_.55.patch queued for re-testing.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

I 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..

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This looks like a great little patch, but it will need a re-roll for #1785256: Widgets as Plugins.

sun’s picture

Status: Needs work » Needs review

Can we do that in a follow-up issue, please? We purposively added a temporary BC layer there.

webchick’s picture

Status: Needs review » Fixed

Oh, 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.

swentel’s picture

Added 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!

catch’s picture

Title: Move Link/URL field type into core » Change notice for: Move Link/URL field type into core
Category: feature » task
Priority: Major » Critical
Status: Fixed » Active

I think we need to add a change notice here for the new module.

swentel’s picture

Status: Active » Needs review

Here's start, should be enough I guess, also added the e-mail field type: http://drupal.org/node/1796546

catch’s picture

Priority: Critical » Normal
Status: Needs review » Fixed

Thanks!

amateescu’s picture

Title: Change notice for: Move Link/URL field type into core » Move Link/URL field type into core
Category: task » feature
klonos’s picture

Has 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.

sun’s picture

I 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.

klonos’s picture

Thanx Daniel. I was already following that issue, but I was looking for one filed against core.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sun’s picture

+/**
+ * Implements hook_field_formatter_info().
+ */
+function link_field_formatter_info() {
+  $formatters['link'] = array(
+    'label' => t('Link'),
...
+  );
+  // @todo Merge into 'link' formatter once there is a #type like 'item' that
+  //   can render a compound label and content outside of a form context.
+  $formatters['link_separate'] = array(
+    'label' => t('Separate title and URL'),
...
+  );
+  return $formatters;
+}

Finally 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

falcon03’s picture

Just 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 :-)

Dave Reid’s picture

yched’s picture

yched’s picture

Issue summary: View changes

Updated issue summary.