Alternate approach: replace tokens on the fly

Quick follow up on this ticket.

After doing quite a few tests, with Commerce Product Reference, Taxonomy Terms, Users or Entity Reference, I came up with the following patch against latest imagefield_tokens-7.x-1.x-dev (2013-01-04), attached to this ticket as:
imagefield_tokens-alternate-approach-replacement-on-the-fly-1934986-1.patch

Please find below a detailed log of the changes:

Detailed log:

In general, this patch is greatly inspired from the patch posted by @james.williams at #1053402-19: ImageField tokens D7 version:

1 - Major change: Removed imagefield_tokens_field_attach_presave and replaced with imagefield_tokens_field_attach_view_alter
Added implementation of hook_field_attach_view_alter, based on initial patch, modified to reduce slightly the code and added inline comments.
The main difference mostly relies in the call to token_replace when the field is being assembled to be displayed. Since token patterns are replaced on the fly, there is no need to store them with an implementation of hook_field_attach_presave, thus removed from code.

2 - Removed alt_field_update_on_edit and title_field_update_on_edit
Since evaluated token values are not stored anymore, but replaced when field is viewed, there is no need to keep the settings or the logic related to update values on edit or storing values.

3 - Code clean-up
Added Doc block comments, inline comments and fixed a few minor coding standard issues.
Added support to Token (dialog) browser UI (since Token 7.x-1.2, for more information see #1872692: Provide support for Token UI browser)

This patch has been thoroughly tested for a while already and seems to work as expected.
(Tested with Features, Internationalization, Entity Translation, Drupal Commerce, Entity Reference, and several other modules, without any conflicts or issues)

If you think the proposed implementation attached would help improving the module and you would consider this feature request as acceptable, I would certainly be glad to discuss further how this code/patch could be integrated to module's code base or any additional work that would contribute to improving the overall quality of the module, for example on documentation.
Another option would be to create another/new branch 7.x-2.x, for example, which would leave the choice to developers and users to choose a preferred structural implementation (features would be the same, with or without storage/replacement on the fly).
Another possibility would be to add a configuration setting to allow users to select whether replacements would have to be stored or take place when field is viewed (allowing both implementations to coexist, for example, by default, it could be a replacement at run-time).

Could you please let me know if I overlooked or missed anything in the current ImageField Tokens module, in terms of whether the approach is correct or not?

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

I would greatly appreciate some help from module maintainers and if any of you could take a bit of time to look into the attached patch to give me your feedback/opinion on this feature request.

Any feedback, testing, changes, recommendations would be highly appreciated.
Thanks to all in advance.

Hi eosrei,

Thanks a lot for your prompt feedback and I'm glad you appreciated the detailed description and ticket.
I'm really happy to see that you would be able to consider this feature request as acceptable and that you would be open to a dialog.

I certainly understand your concerns about compatibility and updating the sites that would be using the current implementation.

I think we could perhaps approach the problem from the following angle:

First of all, I can understand better your initial implementation of hook_field_attach_presave which was probably the closest to a direct Port to D7 of the D6 module and features.
Since hook_field_attach_view_alter was only introduced in Drupal 7 and doesn't exist for Drupal 6, then it could be considered as a new implementation or feature with no backward compatibility (couldn't be ported to D6).

Then it comes down to Drupal 7 with the following possibilities:
a. Creating a new branch (7.x-2.x, for example) would sound perfectly legitimate/reasonable since we could consider the 7.x-1.x branch as a direct port from the 6.x-1.x branch, and 7.x-2.x would be more specific to the Drupal 7 API. The new branch would only keep a single main implementation (based on the most recent hooks/API), in particular, hook_field_attach_view_alter, or at least set the previous implementation as deprecated in the sense that the settings could still be configured but wouldn't be selected by default.
In this case, we could think of adding an implementation of hook_update to the patch for updates of earlier versions.

b. Adding the feature to the existing 7.x-1.x, with the following issues:
- alt_field_update_on_edit and title_field_update_on_edit wouldn't necessarily be useful in the case "View" would be selected.
(We could add a few more JS states '#states' here and there on radios/checkboxes to hide/display)

- Selecting a default: "Save".
I would like to disagree with that: I think an update with the patch (hook_update) could set all existing fields configurations values to "Save", so that nothing changes on update for existing sites.
However, I would personally think that for any new field or configuration added, the default value would have to be "View".
(The idea behind this, would be trying to move forward towards using newer API hooks implementations - hook_field_attach_view_alter).

I would greatly appreciate your feedback on this comment and how you would think we could approach the integration of this feature to the module in the smoothest way.

Feel free to let me know if you would have any questions, comments, issues, objections, recommendations, suggestions or further concerns on any aspects of this feature request, my comment or the attached patch, I would be glad to provide more information, explain in more details or re-roll the patch if necessary.

Any comments, testing, reporting, feedback, reviews or questions would be highly appreciated
Thanks to all in advance for your comments, reviews and replies.
Cheers!

Hi eosrei,

Quick follow-up on this issue:
Since it has already been a little bit more than two weeks without getting any updates on this ticket (see #3 on March 6, 2013), feedback or comments on my latest reply, I was wondering if you had enough time to consider the approaches suggested in #3.

I would greatly appreciate to hear your feedback, questions, comments, concerns or replies on any aspects mentioned above, I would certainly be glad to help improving the patch or module in general.

Once again, any comments, testing, reporting, feedback, reviews or questions would be highly appreciated.
Thanks to all in advance for your comments, reviews and replies.
Cheers!

Hi eosrei,

Thanks again for your kind and prompt answer.

Yet another attempt to follow-up on this issue:
Two more weeks have passed, without getting any updates on this ticket (see #5 on March 21, 2013), feedback, comments or replies to my successive follow-ups, I was wondering if you had enough time to consider the approaches suggested in #3.

Changing priority to Major as an attempt to get more attention from maintainers and other developers.

I would greatly appreciate to hear your feedback, questions, comments, concerns or replies on any aspects mentioned above, I would certainly be glad to help improving the patch or module in general.

Lastly, if this could help on the communication side, feel free to contact me directly at anytime on IRC, perhaps this could help speeding up the discussion process and save some of your time.

Once again, any comments, testing, reporting, feedback, reviews or questions would be highly appreciated.
Thanks to all in advance for your comments, reviews and replies.
Cheers!