Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.For a project I am working on this issue bugged me due to a couple of reasons:
- possible invalidation of caches, not recognizing caches (different urls, for same node)
- "bad" url style when someone right click open the link
- which will lead to bad urls spread by the current user and other modules (share this page for instance)
- issues with too many other modules that rely on a not modified url
Long story short - I implemented it: modified the javascript handling the colorbox and the views integration to use data-attributes instead of the query modification. The modified javascript still fallsback to the former query lookup routine (to not break different implementations not using views ui). If there's need for this feature and the module author is willing to integrate I could provide a patch.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | views output | 64.92 KB | Andre-B |
| #15 | views rewrite | 23.96 KB | Andre-B |
| #6 | colorbox_node-data-attributes-2004310-1.patch | 11 KB | Andre-B |
Comments
Comment #0.0
Andre-Badded the part about fallback
Comment #1
solipsist CreditAttribution: solipsist commentedI agree and it's exactly what I've done as I think using query params is a bit kludgy and poor semantics. Furthermore, it would be very nice to have a Ctools plugin that provides a panels selection rule that can be used set a panel variant that uses a layout plugin that is modal-friendly for serving up nodes when using Panels Everywhere.
Regardless of whether it will be implemented or not, providing a patch is always appreciated in the world of Drupal. So please post the one you made.
Comment #2
metastate CreditAttribution: metastate commentedYes, please post your patch. I don't like the idea of using query parameters either and am running into the bad urls problem you mention in #3 above (AddThis links in my case).
Comment #3
iLLin CreditAttribution: iLLin commented#1 I would like you to prove the possible invalidation of caches, as the URL should always be ran through the "l" function as the system URL. It will fetch the appropriate alias on its on. This module will do nothing to effect that.
#2 Also, I don't modify the URL, so if you right click open, the link will be the right one but with width/height added as query parameters (I get that but I doubt its a problem). Its 2013...
#3 Again, I doubt its a problem as long as your URLS are consistent.
#4 I modify the URL with javascript. But I retrieve the correct path through the menu and fetch the content. I also allow other modules to call hooks against that content to alter it. Not sure what other modules are doing that "rely" on a URL, that seems bad practice. If you could give me some examples that would help.
Note: With this approach, then you couldn't rewrite the field, in views, into a link and change the width/height. There is no such data-attributes in the views rewrite section.
Comment #4
Andre-BI really did not want to start a fight here or go too deep into detail, so I keep this short:
#1 just think of a Varnish for instance, it uses the uri to generate cache ids, so if you have a different uri, you have different caches. end of story
#1.1 do you want to have you urls indexed including the height and width settings? if not you will have to figure out how to do a global redirect to the actual url without the height and width stuff in url (which is +1 request at least, and I doubt that it will work without problems). seconds solution for this would be to use canonical, but why that much work if it cant be solved easily.
#2 this is a different uri, which might interfere with a lot of other things.
#3 think of a sharing button inside of the colorbox, what url will it take? the "good" one, or the "bad" one with that height and width information? this url will get probably spread across different networks then. is it a real problem? depends - for me yes.
#4 I cant, just thinking ahead.
well I guess I got it working though. I will look at it later and diff the module against the current release and submit a patch.
Comment #5
iLLin CreditAttribution: iLLin commentedNot trying to start a fight, trying to have a good discussion as I have to cater to a lot of different experience levels. I tend to lean more on the inexperienced side as the more experience can do work arounds.
I like data-attributes as its cleaner like you suggest. I also firmly believe the internet is going more in that direction, especially with new tags. But, then it won't work easily with the views rewrite field. I could potentially "add to" the views rewrite section a data attributes field (not a bad idea).
If we went data-attributes, that should solve varnish.
#3 it seems we will be trying to share "colorbox/node/1" or w/e url launched. Thats a problem. I am thinking they fetch the URL from somewhere (maybe arg() ? ). I don't use the share this or add this as they are slow so I don't know what they do. We can address that since that is going to be a problem regardless.
What are you thinking as far as data-attributes? data-width, data-height?
Comment #6
Andre-Bpatch against 2.6
Comment #7
metastate CreditAttribution: metastate commentedJust a follow-up with my use case, although it's only partly relevant to this discussion. Maybe it will help other people in the future though, since this issue showed up in a search for AddThis.
I was having problems in line with Andre B's point #3 - in particular, AddThis not getting the proper url from a Colorbox Node popup. It was just sharing the home page as the URL. (I thought this was due to the query parameters at first, but it more likely has something to do with the way AddThis gets the url.)
Anyway, my workaround: I was adding the AddThis code via the node template (not a module) so I found a way to manually tell AddThis what the proper url should be: http://support.addthis.com/customer/portal/articles/125634-setting-the-u...
Comment #8
Andre-Bsure there will always be a way to define the "base" or "canonical" url of a site, but it usually introduces more work as needed :)
Comment #9
Andre-Bany interest on this? patch is there :)
Comment #10
iLLin CreditAttribution: iLLin commentedBeen working on my car lately, I'll get it in soon.
Comment #11
Andre-BI bump on this again, hopefully this will get in soon :)
Comment #12
iLLin CreditAttribution: iLLin commentedI will get to it soon, I promise.
Comment #13
iLLin CreditAttribution: iLLin commentedI modified it to fit 3.x-dev.: http://drupalcode.org/project/colorbox_node.git/commit/f77a86b
Comment #14
Andre-B- cant attach files here, see next comment
Comment #15
Andre-Bthis release is bugged. I guess the change to underscroe in the data attributes name is causing problems: at least in chrome the views output for a rewritten field with data-inner_height and data-inner_width is printed as width and height only:
the rewritten views output was defined as this:
.png)
Also I noticed that the colorbox trigger field in views is not yet using the new data-attributes.
Comment #16
Andre-Bchanging from underscore to dash fixes this for me.
Comment #17
iLLin CreditAttribution: iLLin commentedOK I'll get it updated.
Comment #18
Andre-Bgreat thanks :) hopefully soon, this is critical - if people implement the "broken" way things get worst :/
Comment #19
iLLin CreditAttribution: iLLin commentedCommitted to DEV http://drupalcode.org/project/colorbox_node.git/commit/9b79b97
Not sure what you mean by the views not working right? The views seemed to be working fine for me.
Comment #20
iLLin CreditAttribution: iLLin commentedComment #21.0
(not verified) CreditAttribution: commentedclarification