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

Enable inline PHP syntax highlighting #1141

Merged
merged 1 commit into from Jul 9, 2013
Merged

Conversation

joelclermont
Copy link
Contributor

Pygments has a "startinline" option which is only relevant for the PhpLexer. It allows syntax highlighting without the opening <?php tag. I believe this option is ignored for every other lexer.

You can view the Pygments documentation on it here: http://pygments.org/docs/lexers/ Search the page for startinline to jump right to it.

Could you either set this to true by default using the proposed change? Or could this be exposed as a setting somewhere in _config.yml? I can't think of a reason someone would not want this enabled.

Pygments has a "startinline" option which is only relevant for the PhpLexer. It allows syntax highlighting without the opening <?php tag. I believe this option is ignored for every other lexer.

Could you either set this to true by default using the proposed change? Or could this be exposed as a setting somewhere in _config.yml? I can't think of a reason someone would not want this enabled.
@parkr
Copy link
Collaborator

parkr commented Mar 25, 2013

With this default, can I still write an HTML and PHP-mixed page without getting bitten by this? If I start with a bunch of PHP and include a closing ?> tag and follow it with some HTML, will Pygments highlight the HTML properly?

It'd be great if you could post some examples of how this adds new functionality while maintaining the current functionality. :-)

@joelclermont
Copy link
Contributor Author

Good question. Pygments has a separate HtmlPhpLexer which would likely handle this, but I will do some testing and confirm here shortly.

@parkr
Copy link
Collaborator

parkr commented Mar 25, 2013

Cool, thanks man!

@joelclermont
Copy link
Contributor Author

It looks like it works fine, even with an end tag and mixed HTML content.

Here is the markdown for both blocks (check it in preview, not sure how to show markdown in a github comment without it rendering it. Side note: it renders fine in github too, so they must have startinline set to default here as well)

$eventTime = '2011-09-15';

//add one day to the date 
$newEventTime = strtotime($eventTime . ' +1 day');

//expects 2011-09-16 and will ALWAYS work
echo date('Y-m-d', $newEventTime);
$eventTime = '2011-09-15';

//add one day to the date 
$newEventTime = strtotime($eventTime . ' +1 day');

//expects 2011-09-16 and will ALWAYS work
echo date('Y-m-d', $newEventTime);
?>

<div>some HTML code here</div>

And here is a screen shot of how it is rendered in the default theme:

Screen Shot 2013-03-25 at 10 24 30 AM

@joelclermont
Copy link
Contributor Author

Out of curiosity, I then put some more PHP code after the end tag and HTML. As expected, it does not syntax highlight it as PHP.

Screen Shot 2013-03-25 at 10 33 38 AM

@joelclermont
Copy link
Contributor Author

In my first example, the HTML was not highlighted. So I did some more experimenting. In order to get both PHP and HTML to syntax highlight, you need to use the html+php language, not just php. When I added that language to my code block, even with startinline => true and no opening tag, it worked as expected.

Screen Shot 2013-03-25 at 11 09 01 AM

@parkr
Copy link
Collaborator

parkr commented Mar 25, 2013

Awesome! So you specified exactly html+php to achieve the last example? We'll want to ensure that this lexer is well publicized.

@joelclermont
Copy link
Contributor Author

Yes, no matter what I did with the php lexer it wouldn't also highlight HTML. Looking at the pygment docs I found the html+php language. It says it also parses CSS and JavaScript in the HTML too, which I confirmed with another test.

@parkr
Copy link
Collaborator

parkr commented Mar 25, 2013

Awesome! Let me run this by @imathis, but I'm +1. We get a lot of requests to auto-add PHP tags or whatever but this is easily the best solution.

It looks fine with other languages that don't have such tags, correct? Would you post a quick Ruby and maybe Clojure example just for fun? :-)

@joelclermont
Copy link
Contributor Author

Here is a screenshot of a Ruby and Clojure code block with startinline set to true.

Screen Shot 2013-03-25 at 12 15 22 PM

@joelclermont
Copy link
Contributor Author

This startinline option is only used by the PhpLexer in Pygments, so it should not affect any other languages. They all just seem to ignore it.

@parkr
Copy link
Collaborator

parkr commented Mar 25, 2013

Awesome! Thanks for doing this for us and being so responsive. Once we have @imathis's thoughts, we'll merge in to master and to 2.1.

@joelclermont
Copy link
Contributor Author

No problem. I just started using Octopress this weekend and I'm loving it! Plus, I'm on vacation, so I have time to kill.

There's something else I found, but I'm going to open another issue for your feedback before I dive in and code something. It may be a bit more involved.

@ruflin
Copy link

ruflin commented May 19, 2013

I just found a very strange issue with this and the following code snippets. I'm not sure if this is related to my local setup, a bug or feature" of Pygments.

The two code snippets below are almost identical. The only difference is the s I added to $node->getName(); With the s it works (second example), without the s it doesn't. I can also get it working by replacing the spaces before the echo through a tab. One of my assumption is, that Pygments tries to "understand" the code and tells me the code is not valid, because node $node was not used before (means it doesn't understand the foreach (...) part.). But then also lots of other code should not work. Can anyone of you reproduce this on your side?

$client = new Elastica_Client();

// Retrieve a Elastica_Cluster object
$cluster = $client->getCluster();

// Returns the cluster state
$state = $cluster->getState();

// Gets all cluster notes
$nodes = $cluster->getNodes();

foreach ($nodes as $node) {
    echo $node->getName();
}
$client = new Elastica_Client();

// Retrieve a Elastica_Cluster object
$cluster = $client->getCluster();

// Returns the cluster state
$state = $cluster->getState();

// Gets all cluster notes
$nodes = $cluster->getNodes();

foreach ($nodes as $node) {
    echo $nodes->getName();
}

@joelclermont
Copy link
Contributor Author

Can you post a screenshot of the output for each sample?

@ruflin
Copy link

ruflin commented May 19, 2013

Here we go:
screen shot 2013-05-19 at 15 15 56
screen shot 2013-05-19 at 15 16 24

The non working example is even online here:

http://ruflin.com/2011/11/21/using-elastica-with-multiple-elasticsearch-nodes/

@joelclermont
Copy link
Contributor Author

I tested both your code snippets in my local Octopress and both syntax highlighted with no issue. Are you sure this isn't just a cache issue or a build issue on your end? Pygments definitely does not interpret code, it is just a lexer.

@ruflin
Copy link

ruflin commented May 20, 2013

That was also my first assumption. But the error persists when I rebuild and also for caching. To double check the caching issue:

This page has no code highlighting:
http://ruflin.com/2011/11/21/using-elastica-with-multiple-elasticsearch-nodes/

This page does have code highlight:
http://ruflin.com/2011/11/20/how-to-log-requests-in-elastica/

Is this the same in your browser(s)? I assume for the testing you also made the difference between tabs and spaces?

@joelclermont
Copy link
Contributor Author

The syntax generation is done on your local machine, not in the browser. (But yes, I see one without syntax highlighting and one with.) I would need to see the raw post content for both posts to do any sort of analysis. Can you post them as a separate gist?

@ruflin
Copy link

ruflin commented May 20, 2013

Sure.

Not working (as it is online):
https://gist.github.com/ruflin/b99987448870eb6bba3a

First code example working:
https://gist.github.com/ruflin/19d1f2af438d9e9f8c49

There is some HTML in the posts as I just migrated from an other system. But I would assume, this shouldn't be the issue.

@odino
Copy link

odino commented May 25, 2013

any news on this?

@ruflin
Copy link

ruflin commented May 26, 2013

As it works in 99% of the cases, this is not a big issue for me. But no news from my side so far.

@adrienbrault
Copy link

thanks for your patch

@imathis
Copy link
Owner

imathis commented Jul 9, 2013

This is great for when you want to just write a php only script, but in cases where you have mixed html and php and do not need this option to be turned on, how does that affect the highlighting?

Could you provide a screen shot of this code with and without the option turned on?

<?php  if ($g_user->hasPermission("ManageCountries")) { ?>
<table BORDER="0" CELLSPACING="0" CELLPADDING="1">
    <tr>
        <td><a href="add.php"><?php putGS("Add new"); ?></a></td>
    </tr>
</table>

If it doesn't affect the HTML I'd be happy to accept it as a default, but if it does, I'll work on setting it up as an optional argument. Then it becomes a question of what the default should be. I lean toward letting the dependencies inform on proper defaults, so I'm tempted to say it should be off by default. That way the pygments experience doesn't change a lot. But I could be convinced that it should be changed.

@joelclermont
Copy link
Contributor Author

Looks identical rendered with and without startinline option. Note, the bottom code block in each screenshot is using the html+php lexer and the top block is using the php lexer. (edited - had them backwards)

Without startinline
without startinline

With startinline
with startinline

@imathis
Copy link
Owner

imathis commented Jul 9, 2013

Sweet deal. Thanks.

imathis added a commit that referenced this pull request Jul 9, 2013
Enable inline PHP syntax highlighting
@imathis imathis merged commit 64bb9a3 into imathis:master Jul 9, 2013
@joelclermont joelclermont deleted the patch-1 branch July 9, 2013 15:21
briansimmons pushed a commit to briansimmons/octopress that referenced this pull request Aug 20, 2013
Enable inline PHP syntax highlighting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants