Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hgoto created an issue. See original summary.

hgoto’s picture

Status: Active » Needs review
FileSize
2.04 KB

Here is a first trial patch. I'd like this to be reviewed.

marvil07’s picture

Thanks for the patch, I verified it still works with asciidoc binary.

The patch looks good, except for two minor things:

+  if ($is_command_avaiable) {
     $severity = NULL;
     $version = substr(array_shift($output), 9);
   }

The admin/reports/status page shows asciidoc version parsing output and assuming it is like asciidoc 8.6.9, you may want to change the substr(array_shift($output), 9); line to report the version correctly.

+  $is_command_avaiable = FALSE;

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?

marvil07’s picture

Status: Needs review » Needs work
hgoto’s picture

Status: Needs work » Needs review
FileSize
2.67 KB
1.18 KB
13.19 KB
14.62 KB

@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.)

  • marvil07 committed 473840f on 8.x-1.x authored by hgoto
    Issue #2893855 by hgoto: Add support for asciidoctor
    
  • marvil07 committed 6e20125 on 8.x-1.x
    Issue #2893855: Do not use REQUIREMENT_OK on hook_requirements().
    
    See...
marvil07’s picture

Status: Needs review » Fixed

Thanks 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.

  • marvil07 committed 3df5ed3 on 8.x-1.x
    Issue #2893855: Document new possible asciidoctor requirement.
    
hgoto’s picture

@marvil07 thanks! I didn't notice the points you modified.

I'll open a follow-up issue soon...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.