Module for Drupal 7.x.
Project page: http://drupal.org/sandbox/DYdave/1837132
GIT info:

git clone --recursive --branch 7.x-1.x DYdave@git.drupal.org:sandbox/DYdave/1837132.git image_link_formatter

Module Description:

This module is the result of the discussions around a requested feature to allow an image field to be displayed with a link to a custom URL:

It seems many attempts and implementations have already been proposed...

Read more on module's page.

Already went through Coder module's validation.

Please let me know if you would have any questions on any points/code/aspects mentioned in the project, I would surely be glad to provide more information.

I would greatly appreciate if you could take a bit of time to take a look at the code and give me your feedback on the module.

Thanks in advance.

Reviews of other projects:

[Round1]

  1. Foresight Images: http://drupal.org/node/1860154#comment-6823728
  2. JS Spellcheck: http://drupal.org/node/1860736#comment-6823688
  3. Room Reservations: http://drupal.org/node/1860372#comment-6823632



[Round2: Back for more Review Bonus]

  1. MuseScore: http://drupal.org/node/1857646#comment-6827684 [Manual Review, Long/Detailed]
  2. Custom Add Block: http://drupal.org/node/1846194#comment-6827762
  3. Entity reference feeds: http://drupal.org/node/1862368#comment-6827720

[Round3: Back for more Review Bonus]

  1. Imagefield CSS: http://drupal.org/node/1865074#comment-6847746 [Manual Review, picked up PAReview: security issues]
  2. Taxonomy Publisher Filter: http://drupal.org/node/1862664#comment-6828126
  3. Range field: http://drupal.org/node/1848868#comment-6848276

Comments

anydigital’s picture

anydigital CreditAttribution: anydigital commented
Status: Needs review » Needs work

Hi DYdave,

1) Fix all the issues from automatic reviewer: http://ventral.org/pareview/httpgitdrupalorgsandboxdydave1837132git
2) Add README.txt file to your module.
3) As I understand your module duplicates the functionality of core image module and uses the Link module to provide links for images. May be you should to suggest your changes to be included in Link module?

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Status: Needs work » Needs review

Thank you very much tonystar for your speedy reply.
It is highly appreciated.

Sorry about 1) and 2)... I could have anticipated that....
I went through the validation again, with the link in your reply and fixed all issues (See related commits).

Now, concerning 3):

3) As I understand your module duplicates the functionality of core image module and uses the Link module to provide links for images. May be you should to suggest your changes to be included in Link module?

I'm going to place a feature request in the Link module, but I can already foresee several issues:

a. The most immediate issue is that I'd like to create a new branch for this module: 2.x to integrate with: Field Formatter Settings (See: #945524: Field formatter settings hooks in core), which would be adding a dependency on the module and I would assume that it would most likely be a problem if this had to be integrated to the Link module: people will argue this dependency is not really necessary.

b. Another issue I can see is also people complaining about only having a single formatter included in the module... some might argue: Why wouldn't we be adding another formatter for textual fields, for example?... then we're back to the Linked Field method, which is probably not going to be practical for some users, who would only need this feature for images.

c. The last issue I thought of would be the maintenance of the changes to the image formatter from core (since this is a copy over, in 1.x branch), if we were to include the 1.x branch of this module, in the Link module. Some might argue again, this is more maintenance work for them that should probably be kept outside of the module, on its own.

Therefore, initially, I had assumed this feature could be added as a stand alone module in order to avoid any complications due to discussions related with the Link module.
But for sure, I will bring this discussion topic to the Link module and as a result, either the module (1.x) will be integrated, leaving only the 2.x branch for those accepting the dependency on Field Formatter Settings, and/or perhaps add a link with some small help text on the Link module's description page to mention about the existence of this particular formatter.

Once again, I would greatly appreciate your feedback on this.

Thanks again very much for your help and comments.

grisendo’s picture

grisendo CreditAttribution: grisendo commented
Status: Needs review » Needs work

There is a XSS security issue. If a put:
<script>alert('XSS');</script>
as the label of the Link field used, in the display page (in the summary) that JS code is executed.

Please, sanitize it using filter_xss_admin, i.e. in line 77 instead of:
$summary[] = $link_types[$settings['image_link']]);
use:
$summary[] = filter_xss_admin($link_types[$settings['image_link']]);

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Issue tags: +PAreview: security

Thanks a lot grisendo for your valuable feedback.

I've updated the module according to your recommendations at: ebf4986 (diff).

Feel free to let me know if you would see any other security issue or problem in the code or modules in general, I would surely be glad to answer.

Testing feedback and comments are always highly appreciated.
Thanks again.

queenvictoria’s picture

queenvictoria CreditAttribution: queenvictoria commented
Status: Needs work » Needs review

I've got another use case for your module and have created another sandbox project to develop it. I wonder if they are common enough to merge into one in future? Let me know what you think. I'll review your module in the meantime.
Actually reading more about it they are separate ideas.

jenyum’s picture

jenyum CreditAttribution: jenyum commented

Hi DYDave,

Thanks so much for putting this together! I haven't had a chance to tinker with this yet, but could you tell me if it works when multiple image values/multiple link values are permitted on a content type?

For example, if I have a node with 10 image fields and 10 link fields, will the links and images match up and can I create them together so that the assignment makes sense? (there were some work arounds that could produce a custom link on an image without this project, but none would work with multiple image fields attached to the same node.)

DYdave’s picture

DYdave CreditAttribution: DYdave commented

Hi jenyum,

Thanks very much for your comment and I'm really glad you're finding an interest in this module.

I'm also very grateful that you've put your finger on one of the most important use cases for this module: being able to combine link and image fields with multiple values.

So this is something I tried to explain in more details on the module's page, in the part for Important notes:

c. Multiple values:
Multiple field instances will work based on field delta.
For example: value 0 of field link will be wrapped around value 0 of field image. Value 1 of field link will be wrapped around value 1 of field image .... and so forth.

Being a developer myself, the terms used for this explanation were perhaps a little bit too technical, so let me try to use your question to describe module's behavior in other words:

For example, if I have a node with 10 image fields and 10 link fields, will the links and images match up and can I create them together so that the assignment makes sense?

Yes, definitely, this is exactly what the point Important note c. (quoted above) means.
Now, this also means that it will only work with this configuration if there is no gap in the sequence of values. Otherwise, it won't work with this method and you would have to use another way (with field_collection, for example, explained further below). So for example, you can't have:
image0 | link0
image1 | [empty]
image2 | link2
.....
The sequence has to be continuous, there can't be any gap.
So in this case, if the second value was left empty, then link2 would be displaying on image1, no link on image2 and the sequence would be broken.

Another solution, as explained on the module's page, would be to use the field_collection module.
In this case, the multiple values feature is actually provided by the field_collection.
Image and Link fields would have to be set to single values, then the field collection would be multiple.
Upon display configure the formatters for the link and image for the field collection display.
With this solution, any sequence of links would potentially be possible whether the link field has a corresponding value or not.

I hope I was able to answer your questions, but please let me know if I overlooked or missed anything anything.

Feel free to let me know if you would have any other questions, comments or issues, I would surely be glad to explain in more details or provide more information on specific functions.

Thanks again very much for your feedback and testing.

DYdave’s picture

DYdave CreditAttribution: DYdave commented

@queenvictoria:
Too bad it didn't work out with this module, but thanks a lot for having taken the time to post here and look further into this module.

Feel free to post back if you have any other questions, comments or issues with this module, I would surely be glad to answer.
Thanks!

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Issue tags: -PAreview: security +PAreview: review bonus

Added 3 reviews to issue summary.
Adding PAReview: review bonus tag.
Removed PAReview: security tag: Security issues have been resolved according to what was suggested above, see: #3.

Moving on to the fast lane: wiiiiiiiiii!!

Thanks again for all your comments and feedbacks!
Cheers!

anwar_max’s picture

anwar_max
Primary language English
CreditAttribution: anwar_max commented
Issue tags: +PAreview: security

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

anwar_max’s picture

anwar_max
Primary language English
CreditAttribution: anwar_max commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

Automated Review:

Review of the 7.x-1.x branch:

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.


FILE: ...es/all/modules/pareview_temp/test_candidate/image_link_formatter.module
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 124 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 124 | ERROR | Line indented incorrectly; expected 6 spaces, found 3
 125 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 125 | ERROR | Inline control structures are not allowed
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Manual Review:

Everything is looking good :)

Removing Review bonus tag

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Status: Needs work » Needs review

Thanks very much anwar_max for the review, and again to everyone else.

Indeed, I changed a few lines and introduced a validation error, in particular: Inline control structures are not allowed.

I fixed all reported errors again, which was pretty fast and automated review seems to be fine again now:
PAReview report

According to previous comment and with these fixes from the automated review, everything should be fine now:
- Manual review [Passed, see: #11]
- Automated review [Passed, see: PAReview report]

Please let me know if you would have any questions on any points/code/aspects mentioned in the project, I would surely be glad to provide more information.

Thanks in advance.

@anwar_max: #10
Sorry for removing the tag, I wasn't really aware of what it would be used for.
Thanks for making it clear.

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Issue summary: View changes

Adding new section to issue summary: Reviews of other projects

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Issue tags: +PAreview: review bonus

Added 3 reviews to issue summary (See Round2, total 6 reviews collected so far).
Adding PAReview: review bonus tag.

Back on to the fast lane: wwwwiiiiiiiiiiiiiiii!!!!!!

Thanks again for all your comments and feedbacks!
Cheers!

gigabates’s picture

gigabates CreditAttribution: gigabates commented

Hi DYdave.

Everything looks good to me on the current version so no problems to report.

I do have a question about you approach here though. Did you consider overriding the core Image formatter rather than adding an additional formatter? As far as I'm aware this would be possible. As this module mirrors the core functionality with the addition of links integration I wondered whether this would be a better approach.

DYdave’s picture

DYdave CreditAttribution: DYdave commented

Hi gigabates,

Thanks very much for your comment and I really appreciate your feedback and approach.

I'm afraid, I'm not exactly clear with what you mean by:

Did you consider overriding the core Image formatter rather than adding an additional formatter?

Because the point of the module here is not to completely override the default core Image formatter, but more to provide another formatter, along with the default.

Now, there would indeed, be an approach and a solution to do what you suggest, but it would probably require the Field formatter settings module, see #2.a in this post, in particular:

The most immediate issue is that I'd like to create a new branch for this module: 2.x to integrate with: Field Formatter Settings (See: #945524: Field formatter settings hooks in core), which would be adding a dependency on the module and I would assume that it would most likely be a problem if this had to be integrated to the Link module: people will argue this dependency is not really necessary.

This would allow providing a checkbox on the core image field formatter settings form(See: #945524: Field formatter settings hooks in core) to activate or not the linking functions.
Then, by checking the checkbox a dropdown would display underneath to let the user select the corresponding link field attached to the entity bundle.
I have already thought of an idea like that, but in any case, the module should leave the user the option to enable/disable this behavior and not totally override the core Image formatter.

I would certainly be very happy to hear your feedback and comments about that, since this would be directly related with a potentially planned 2.x Branch (see module's page, the part that says future developments :-)).
For now, 1.x Branch only depends on link and image modules and adds an extra formatter.

Please let me know if I misunderstood anything from your last comment, I would certainly be glad to have some clarifications about what you meant in terms of coding and try to reply more accurately or with more information.

It would certainly help greatly improving the module by finding a more appropriate solution for this very common use case.

Thanks again very much for testing and feedback.
Cheers!

gigabates’s picture

gigabates CreditAttribution: gigabates commented

Because the point of the module here is not to completely override the default core Image formatter, but more to provide another formatter, along with the default.

Yeah I can see that's the approach you've taken but I was just wondering if there's any need to offer two formatters, with and without the additional options. Obviously your approach works great, I just thought it was worth raising.

DYdave’s picture

DYdave CreditAttribution: DYdave commented

Thanks a lot gigabates, once again.

What I thought I would be doing is try to leave the choice to users to choose to work with either:

  • Branch 1.x: only depends on Link and Image modules and adds an extra formatter.
  • Branch 2.x: depends on: Field formatter settings, Image and Link modules. From a maintainer's stand point (pretty much me :-) ), I would prefer maintaining the 2.x branch since no code would have to be duplicated/mirrored from Core Image module's formatter. Something similar to/inspired from what Linked Field module does.

So this module, as it is currently (7.x-1.x Branch) will need more work for maintenance (at least some verification) upon Drupal Core updates.

Feel free to post a ticket in project's tracker if you ever come up with an idea to better achieve the use case and concepts discussed on project's page.

Thanks again very much for your comments, feedbacks and interest.
Cheers!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Did a manual review, looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue summary: View changes

Adding second round of reviews.

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Issue tags: +PAreview: review bonus

Added 3 reviews to issue summary (See Round3, total 9 reviews collected so far).
Adding PAReview: review bonus tag.

Back on to the fast lane: wwwwiiiiiiiiiiiiiiii!!!!!!

Thanks again for all your comments and feedbacks!
Cheers!

DYdave’s picture

DYdave CreditAttribution: DYdave commented

Just a quick followup:

I wouldn't want to sound insistent or anything, but would there be any blocking remaining things/issues that would prevent this module from getting out of sandbox?

Feel free to let me know if you would have any questions, comments or issues on any aspects of this project or ticket, I would surely be glad to explain in more details.

Thanks again to all of you for your feedback and comments.
Cheers!

stBorchert’s picture

stBorchert
he/him
Primary language German
CreditAttribution: stBorchert commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, David!

I wouldn't want to sound insistent or anything, but would there be any blocking remaining things/issues that would prevent this module from getting out of sandbox?

Yes, unfortunately there are dozens of applications and only a few git administrators who can do a final review :/

Nevertheless I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project :).

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

DYdave’s picture

DYdave CreditAttribution: DYdave commented

Thanks very much to everyone who worked on this project application.

Thanks stBorchert for the very useful information for the maintenance and best practices on administering modules.

Module is finally out of sandbox and landed in its intended nest: Image Link Formatter.

Feel free to report any issues, feature requests or bugs, in the tracker, I would certainly follow up as soon as possible.

I'm looking forward to getting more feedback and improvements on the module.
Thanks again to all.
Cheers!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Adding third round of reviews.