Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to enable source highlighting of pure PHP code #755

Closed
mojavelinux opened this issue Feb 15, 2017 · 9 comments
Closed

Add option to enable source highlighting of pure PHP code #755

mojavelinux opened this issue Feb 15, 2017 · 9 comments
Assignees

Comments

@mojavelinux
Copy link
Member

mojavelinux commented Feb 15, 2017

By default, both Pygments and Rouge require PHP tags, <?php and ?>, to surround the code in order to highlight it. This requirement can be dropped by setting the "start inline" option (startinline for Pygments, start_inline for Rouge). We need a way to express this setting in AsciiDoc.

Introduce the option startinline to the source block to enable the "start inline" option for PHP highlighting.

[source%startinline,php]
----
echo "Hello world";
----

UPDATE: We're now proposing the inverse setting as the default. See comments below.

@mojavelinux
Copy link
Member Author

An open question is whether we should enable this by default. Are there use cases for which enabling this option breaks highlighting?

@mojavelinux mojavelinux changed the title Add option to enable source highlighting of inline PHP Add option to enable source highlighting of pure PHP code Feb 15, 2017
@mojavelinux
Copy link
Member Author

To answer that open question, should not. If you have mixed HTML and PHP, then the highlighting will break when this option is enabled.

Here's an example:

[source,php]
----
<p>This echo is going to be ignored by PHP and displayed by the browser.</p>
<?php echo 'This is going to be parsed.'; ?>
<p>This echo will also be ignored by PHP and displayed by the browser.</p>
----

(Interesting to note that at the time of this issue report, GitHub does not properly highlight that snippet, so GitHub must be running with the "start inline" option on by default).

@mojavelinux
Copy link
Member Author

I'm inclined to invert this option. We use the "start inline" option by default and require the "mixed" option to revert it back.

[source%mixed,php]
----
<p>This echo is going to be ignored by PHP and displayed by the browser.</p>
<?php echo 'This is going to be parsed.'; ?>
<p>This echo will also be ignored by PHP and displayed by the browser.</p>
----

I think that makes the intent more clear.

@mojavelinux
Copy link
Member Author

If you include a full PHP file, it will be necessary to set the mixed option, even though the only mixed part about it is the surrounding PHP tags. But technically, it's still true.

mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Feb 15, 2017
…ng PHP by default

- enable "start inline" option when highlighting PHP by default
- disable setting if the mixed option is set on source block
mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Feb 15, 2017
…ng PHP

- enable "start inline" option when highlighting PHP by default
- disable setting if the mixed option is set on source block
@lucascosti
Copy link

I don't do a lot of PHP any more, but I would be inclined to have startinline enabled by default.

@mojavelinux
Copy link
Member Author

Great, that's the direction we're headed.

From the limited research I did, I think it's clear that it must be possible to control this setting or else you'll end up in situations where mixed code isn't highlighted correctly. But the mixed mode seems to be the exception (for snippets), hence why we want to start with it enabled.

@mojavelinux
Copy link
Member Author

I'm definitely interested to hear from people who disagree with that statement and why.

@akrabat
Copy link

akrabat commented Feb 15, 2017

As a PHP developer, I would prefer that the default is start inline as modern PHP code is separated from HTML, unless it's output specific logic and even then it's fairly common to use DSLs like Twig.

@mojavelinux
Copy link
Member Author

Great, I think we're on the right track then.

Is everyone okay with using the name mixed for the option that disables this switch?

I'm going to proceed with merging, but there is still time to revise before release, if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants