I use Drupal-6 with simplenews to send out notes from my church's Sunday morning worship services. I use ordered-lists to quote scriptures, and had to modify the standard drupal_html_to_text function to support the value attribute of ordered lists, else my scripture-quotations would come through (in the text version) with the wrong verse numbers.
Attached one-line patch adds support for the value attribute, so that the html_to_text function converts
<ol>
<li value=5>This is the text for item number five</li>
</ol>
to
5. This is the text for item number five
instead of converting it to
1. This is the text for item number five
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | includes_mail.inc_.patch | 1.27 KB | pillarsdotnet |
| #12 | includes_mail.inc_.patch | 823 bytes | pillarsdotnet |
| #11 | includes_mail.inc_.patch | 823 bytes | pillarsdotnet |
| #9 | includes_mail.inc_.patch | 817 bytes | pillarsdotnet |
| mail.inc_.drupal_html_to_text.ordered-lists-value-attribute.patch | 789 bytes | pillarsdotnet |
Comments
Comment #1
stevenpatzfeatures go in HEAD
Comment #2
pillarsdotnet commentedYes, I'm aware of that, but I'm unwilling to run 7.x on a production website. Meanwhile, this particular one-line change has been working successfully on two production websites for over a month with no detectable problems. Setting version back to "6.x-dev" because I suspect the patch won't apply against HEAD anyway.
Setting category to "Task" because there doesn't seem to be an appropriate category for "useful fix for a non-critical problem that i want to share but don't expect anyone in the core team to actually implement within the next year or two."
If you're running a production site on 7.x and want to re-roll and test the patch against HEAD, feel free to change the version back, in which case I'll open up a new issue for a 6.x backport.
I'm not trying to get into an argument or ruin anyone's day. I just feel compelled to share a solution to this problem in case anyone else is interested. If there is a simple way to solve the same problem without changing a file in core, please enlighten me.
Comment #3
stevenpatzComment #4
pillarsdotnet commentedComment #5
damien tournoud commented@pillarsdotnet: this will need to be discussed and reviewed in 7.x, before even being considered for backport to D6. Please do not change the version again, or you request will never be considered.
Comment #6
dries commentedPatch has coding style issues; not conform the code guidelines. Personally, I'm tempted to mark this 'postponed' until more people requested this.
Comment #7
pillarsdotnet commented@Dries etc.
All of which is exactly why I marked it 6.x rather than 7.x.
I'm not willing to install 7.x just so I can test a one-line patch, because I am quite sure that it would require days of work just to get the rest of the system into a usable state before my insignificant little change would even matter.
However, in 6.x, which is largely usable already, my patch solves a real-world problem.
Since I'm using open-source software, I feel obligated to contribute my fixes back to the community, however small and insignificant.
Obviously, submitting a patch into to the Drupal project was the wrong way to contribute this particular solution back to the community.
Would you please direct me to a proper and acceptable way to contribute this particular solution back to the community?
Comment #8
webchick@pillarsdotnet: Nope, your approach is right on, and it's fine to upload patches that are intended for 6.x. It's just that in order for this to get into Drupal core, we need to first fix it in the latest version (in this case, HEAD/7.x) and then back-port it. This is so that we don't get weird version idiosyncrasies where some stuff is fixed in 6.x, others in 5.x, and still others in 7.x. There needs to be some method to the madness, and so "fix it in the latest version and backport as appropriate" is the standard the community has found works well for us.
Now, you personally don't have to port this to 7.x. You can wash your hands and be done with this issue right now, if you want. You've already contributed something valuable: you've identified a shortcoming in Drupal, provided a test case, and have supplied a fix that works for you that others can use.
However, someone does have to make this work in 7.x in order for it to be integrated into Drupal core itself. There are two options:
Either option is just as viable. The first one will probably see this in core more quickly.
As a review of the patch itself, there are some coding standards issues, as Dries pointed out. I also wonder if we shouldn't make this an option anywhere we deal with lists, rather than just in mails. For example, theme_item_list and similar. It seems like user expectation would be for the same code to work the same regardless of where it was displayed.
Comment #9
pillarsdotnet commentedRe-rolled for d7.
Comment #11
pillarsdotnet commentedBah. typo.
Comment #12
pillarsdotnet commentedBah. typo.
Comment #13
pillarsdotnet commentedNote that while the li:value attribute is marked as deprecated in html4, it is no longer marked as deprecated in the html5 draft.
@webchick -- I don't understand why there would need to be any corresponding change in theme functions. Since none of them output text or convert html to text, the ol:start and li:value attributes are implicitly supported.
Updated patch adds support for the "start" attribute.
Comment #14
pillarsdotnet commentedMerging with: #299138: Improve \Drupal\Core\Utility\Mail::htmlToText()