Posted by sun on July 7, 2012 at 5:18am
6 followers
Jump to:
| Project: | Link |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
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
| Attachment | Size |
|---|---|
| link.nice_.0.patch | 140.13 KB |
Comments
#1
See also #1674290: PSR-0 tests in contrib modules are not found
#2
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
#3
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.
#4
From #501434-40: Move Link/URL field type into core and following:
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.
#5
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.
#6
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.