Refactoring and improvements from 8.x-1.x.

These patches are essentially cherry-picked commits from the 8.x-1.x branch. The first 2-3 are required to make others apply cleanly. But aside from that, all commits to the 8.x-1.x branch have been kept "atomic"; i.e., compatible for cherry-picking.

These are just basic code improvements, retaining the module's current functionality.

Much more to come... If you're interested in helping, ping me or comment on #1668360: Core developer git access for 8.x-1.x

CommentFileSizeAuthor
link.nice_.0.patch140.13 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

andypost’s picture

It's hard to review a patch-set

+++ b/link.moduleundefined
@@ -799,35 +809,6 @@ function link_migrate_api() {
-function link_cleanup_url($url, $protocol = 'http') {

This function is removed but there's a lot of references in code comments

sun’s picture

Hm. I thought I grepped for the function names that got refactored. I will double-check that, thanks :)

FWIW, yes, it's probably much easier to review the actual commits:
http://drupalcode.org/project/link.git/shortlog/refs/heads/8.x-1.x

As mentioned in the OP, this patch is nothing else than a cherry-pick of commits from 8.x-1.x onto 7.x-1.x.

I'm aware that the lack of testbot is quite a barrier right now. 8.x-1.x doesn't run tests at all (due to #1), and 7.x-1.x has a branch test failure due to a fatal error in one of the tests. I will try to find solutions for that.

sun’s picture

Title: Backport 8.x-1.x improvements » Backport 8.x-1.x into 7.x-2.x + link_advanced module?
Category: task » feature

From #501434-40: Move Link/URL field type into core and following:

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.

In other words, 7.x-2.x would be direct backport of a D8 core module, and thus be "feature-locked." It would only see improvements that also went into the D8 core module.

If there's desire to keep the advanced features that have been removed from the 8.x-1.x branch, then it would probably make sense to create a new link_advanced module that provides them, cleanly separated.

That said, it probably makes sense to postpone this on the actual/final commit of #501434: Move Link/URL field type into core, so there's no moving target.

Furthermore, it's possible that #1778858: Decide on built-in support for internal URLs will be moved into the core queue with the goal of "re-introducing" support for internal links (which has been completely removed from D8 version). It would probably be safer to wait for that, too.

jcfiala’s picture

Neat. I hope I'll be able to take a look at that soon. I'm particularly interested in hearing more about how I should be better arranging the code for Fields. As you probably can tell, I didn't find the documentation for the field api very helpful for D7.

I'll note that there's already an 'advanced_link' module, so creating a link_advanced module could be confusing. It might be useful talking with advanced_link's maintainer about expanding for D8.

jcfiala’s picture

Status: Needs review » Needs work

How, huge patch!

Huge patch that doesn't actually apply anymore - there's been too much drift since it's been created.

I'm slowly going through and doing bits by hand, but there's more to do than I expected, so I don't think I'll get a lot of this in, although some of it (the bringing code up to standards section) is applied.

jcfiala’s picture

Issue summary: View changes

Updated issue summary.

DamienMcKenna’s picture

Issue summary: View changes
Status: Needs work » Closed (won't fix)

I think it's a little late at this point, but thank you for the effort.