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.I don't see any reason why _addtoany_create_button() should be underscore prefixed and labelled an internal function.
It is a very valid requirement to want to add one of these buttons via some custom code but this is discouraged with the internal designation.
Unless there is a good reason why it shouldn't be called directly I think it should be changed to addtoany_create_button().
In addition to that, since an addtoany button is in no way restricted to nodes, the main function shouldn't be taking a node variable.
To address this I think this function should only taking in params required to make a generic button, eg. link name, url, etc., then have a second function for addtoany_create_node_button(), which takes the node param and pulls what it needs from there then calls addtoany_create_button().
This would give developers easy access to both addtoany_create_node_button() and addtoany_create_button() functions and possibly in future ones for other entities.
On that note check out the entity_uri() and entity_label() functions for a potentially generic entity solution.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | addtoany-refactor_button_create-2542488-6.patch | 9.74 KB | micropat |
Comments
Comment #1
micropat CreditAttribution: micropat commentedTotally agree. In addition, it makes sense to pluralize
addtoany_create_buttonasaddtoany_create_buttons.Patches are welcomed.
Comment #2
rooby CreditAttribution: rooby commentedHere is a patch that refactors the _addtoany_create_button() function and the calls to it.
Comment #3
rooby CreditAttribution: rooby commentedWhoops. There was a typo in one of the comments. Updated patch.
Comment #4
rooby CreditAttribution: rooby commentedActually this needs a little more work.
check_plain($link_name)is called in _addtoany_create_script() but sometimes that variable has already run through check plain so you may get double encoding._addtoany_create_script() should assume that the value has already been sanitised.
That will be OK because it has been designated an internal function.
Then addtoany_create_script() should check_plain() for any $title passed in or the site name variable.
Functions like page_title_page_get_title() and drupal_get_title() are already escaped.
Comment #5
rooby CreditAttribution: rooby commentedNew patch
Comment #6
rooby CreditAttribution: rooby commentedActually I'm wondering if it should check_plain() the title.
It's probably better to require developers to pass in a sanitised value.
This is because people are going to sometimes need to pass in already sanitised values.
Comment #7
rooby CreditAttribution: rooby commentedWith this change, #2551215: Replace node functionality with addtoany field and #2551213: Change addtoany block to work for all pages, not just nodes I wonder if all those together should form a new 5.x version?
I'm happy to write patches for those other 2 issues also.
Comment #8
micropat CreditAttribution: micropat commentedPushing to dev this new patch, which fixes an error and notice (with an active block) in patch 5, and also fixes addtoany_handler_field_addtoany_link.inc to use new addtoany_create_node_buttons(). Parameters are in order of importance, so usage will be like:
addtoany_create_buttons($url, $title);.Let's keep the function optimized for sanitization and the most common case. In cases where a value is already encoded, unencode it before passing it to addtoany_create_buttons() like:
html_entity_decode(strip_tags($title), ENT_QUOTES)5.x is a good idea. Altogether, these changes could alter the expected results of some workarounds in the wild.