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.
Thank you for the useful module. I'd like to add a support for Asciidoctor.
Comment | File | Size | Author |
---|---|---|---|
#5 | 2893855-5-asciidoctor.png | 14.62 KB | hgoto |
#5 | 2893855-5-asciidoc.png | 13.19 KB | hgoto |
#5 | interdiff-2893855-2-5.txt | 1.18 KB | hgoto |
#5 | asciidoc-add_support_for_asciidoctor-2893855-5.patch | 2.67 KB | hgoto |
Comments
Comment #2
hgoto CreditAttribution: hgoto as a volunteer commentedHere is a first trial patch. I'd like this to be reviewed.
Comment #3
marvil07 CreditAttribution: marvil07 as a volunteer commentedThanks for the patch, I verified it still works with asciidoc binary.
The patch looks good, except for two minor things:
The
admin/reports/status
page shows asciidoc version parsing output and assuming it is likeasciidoc 8.6.9
, you may want to change thesubstr(array_shift($output), 9);
line to report the version correctly.Typo.
I really have never used asciidoctor, but it looks like it supports the same arguments, so it should be OK to add it here.
Possible Future changes
One thing that would be great would be a way to choose among the two if the two are installed.
With the patch it chooses the first it finds prioritizing asciidoc binary, so behaviour is maintained, that's good.
I think the right place to do that is on the filter configuration, currently is it empty.
@hgoto Do you want to open a follow-up issue about that?
Comment #4
marvil07 CreditAttribution: marvil07 as a volunteer commentedComment #5
hgoto CreditAttribution: hgoto as a volunteer commented@marvil07 thank you for your response!
I fixed the points you noted. And I added a small change to the requirements message so that users would know asciidoctor can be used.
As you guess, asciidoctor supports the same argument as asciidoc does as far as I checked.
The possible future changes sound good. I'll open a follow-up issue and give it a try after this issue is closed.
(I attached 2 capture images of the status page as well a patch and an interdiff file.)
Comment #7
marvil07 CreditAttribution: marvil07 as a volunteer commentedThanks for the changes, this is now part of 8.x-1.x.
I did one minor change after adding it. See the following change notice: Removed REQUIREMENT_INFO/REQUIREMENT_OK from most hook_requirements() items for details.
Comment #9
hgoto CreditAttribution: hgoto as a volunteer commented@marvil07 thanks! I didn't notice the points you modified.
I'll open a follow-up issue soon...