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

Mathjax preprocessing no longer working in PB 4.x #958

Closed
monkecheese opened this Issue Oct 3, 2017 · 14 comments

Comments

Projects
None yet
3 participants
@monkecheese
Contributor

monkecheese commented Oct 3, 2017

Prerequisites

Check the boxes below by changing them from [ ] to [x].

Description

We are using a plugin that uses the action/filter hooks created in PB 3.9.7 to drop in our own latex renderer (MathJax), see example. Now, though MathJax is selectable as a renderer, we were also loading some preprocessing scripts that are no longer working.

Steps to Reproduce

  1. Install/enable mathjax plugin
  2. drop in the suggested sample latex on a page: [latex]e^{\i \pi} + 1 = 0[/latex]

Expected behavior: The second attached image

Actual behavior: The first attached image

System Information

Pressbooks: 4.3.2
WordPress: 4.8.2
PHP: 7.0

screen shot 2017-10-03 at 12 23 31 pm

screen shot 2017-10-03 at 12 24 22 pm

@monkecheese

This comment has been minimized.

Show comment
Hide comment
@monkecheese

monkecheese Oct 3, 2017

Contributor

A workaround solution for us is to wrap your doThisShortcode (on line 46 in pb-latex.php) in a conditional that checks to see if the pb_enqueue_latex_scripts is called:

if ( ! has_action( 'pb_enqueue_latex_scripts' ) ) {
	add_filter( 'the_content', array( &$this, 'doThisShortcode' ), 8 ); // Before wpautop()
}

I'm not sure if you guys will want this in your code base or not, but I have this solution implemented in our sandbox instance, and would be happy to submit a PR if this solution works for you. If not, I'm also happy to discuss other possibilities. Cheers! :)

Contributor

monkecheese commented Oct 3, 2017

A workaround solution for us is to wrap your doThisShortcode (on line 46 in pb-latex.php) in a conditional that checks to see if the pb_enqueue_latex_scripts is called:

if ( ! has_action( 'pb_enqueue_latex_scripts' ) ) {
	add_filter( 'the_content', array( &$this, 'doThisShortcode' ), 8 ); // Before wpautop()
}

I'm not sure if you guys will want this in your code base or not, but I have this solution implemented in our sandbox instance, and would be happy to submit a PR if this solution works for you. If not, I'm also happy to discuss other possibilities. Cheers! :)

@connerbw

This comment has been minimized.

Show comment
Hide comment
@connerbw

connerbw Oct 4, 2017

Member

Was this working before? If yes, I suspect this PR broke things for you:

https://github.com/pressbooks/pressbooks/pull/923/files

Can you review what part needs to be put back?

Member

connerbw commented Oct 4, 2017

Was this working before? If yes, I suspect this PR broke things for you:

https://github.com/pressbooks/pressbooks/pull/923/files

Can you review what part needs to be put back?

@connerbw

This comment has been minimized.

Show comment
Hide comment
@connerbw

connerbw Oct 4, 2017

Member

Another thing that changed, we synced the code to WP trunk (tried to make it look more like this)

Commit is here: 763c7de

Of note, this:

		add_filter( 'the_content', array( &$this, 'inlineToShortcode' ), 8 );

Changed to this:

		add_filter( 'the_content', array( &$this, 'inlineToShortcode' ), 7 ); // Before wptexturize()
		add_filter( 'the_content', array( &$this, 'doThisShortcode' ), 8 ); // Before wpautop()

Not sure...

Member

connerbw commented Oct 4, 2017

Another thing that changed, we synced the code to WP trunk (tried to make it look more like this)

Commit is here: 763c7de

Of note, this:

		add_filter( 'the_content', array( &$this, 'inlineToShortcode' ), 8 );

Changed to this:

		add_filter( 'the_content', array( &$this, 'inlineToShortcode' ), 7 ); // Before wptexturize()
		add_filter( 'the_content', array( &$this, 'doThisShortcode' ), 8 ); // Before wpautop()

Not sure...

@monkecheese

This comment has been minimized.

Show comment
Hide comment
@monkecheese

monkecheese Oct 4, 2017

Contributor

Thanks @connerbw. I checked out the first code you linked to and tested. Looks like it was broken at that point. The second code you linked to seems to be the culprit. Because when I remove this line, everything fires up and works as expected:

add_filter( 'the_content', array( &$this, 'doThisShortcode' ), 8 ); // Before wpautop()
Contributor

monkecheese commented Oct 4, 2017

Thanks @connerbw. I checked out the first code you linked to and tested. Looks like it was broken at that point. The second code you linked to seems to be the culprit. Because when I remove this line, everything fires up and works as expected:

add_filter( 'the_content', array( &$this, 'doThisShortcode' ), 8 ); // Before wpautop()
@connerbw

This comment has been minimized.

Show comment
Hide comment
@connerbw

connerbw Oct 4, 2017

Member

@monkecheese

Your code, was it forked from here?

Your header says 1.1. Upstream (?) says 1.3.7. They don't look the same.

I don't want to send you on a wild goose chase, but I'm worried about changing this code. It's copy/paste from WordPress. They claim this change was a bugfix to "Fix interactions between LaTeX, wptexturize(), and wpautop()." (Source)

Is it possible that something in plugins/mathjax-latex can be copied over to fix the issue?

Member

connerbw commented Oct 4, 2017

@monkecheese

Your code, was it forked from here?

Your header says 1.1. Upstream (?) says 1.3.7. They don't look the same.

I don't want to send you on a wild goose chase, but I'm worried about changing this code. It's copy/paste from WordPress. They claim this change was a bugfix to "Fix interactions between LaTeX, wptexturize(), and wpautop()." (Source)

Is it possible that something in plugins/mathjax-latex can be copied over to fix the issue?

@monkecheese

This comment has been minimized.

Show comment
Hide comment
@monkecheese

monkecheese Oct 4, 2017

Contributor

@connerbw

The problem is in the creation of the shortcode here. The config script that we load here is looking for [latex] ... [/latex] on the page, and instead it's finding the <img src='$url' alt='$alt' title='$alt' class='latex' /> being built by the shortCode function.

Another possibly better solution might be to add a new filter that allows us to return our own shortcode (bypassing the shortCode function) in favor of our own that allows mathjax to render properly.

Example: (pb_add_latex_shortcode)

function doThisShortcode( $text ) {
	$current_shortcodes = $GLOBALS['shortcode_tags'];
	remove_all_shortcodes();

	add_shortcode(
		'latex',
		apply_filters( 'pb_add_latex_shortcode', [ $this, 'shortCode' ] ) );

	$text = do_shortcode( $text );

	$GLOBALS['shortcode_tags'] = $current_shortcodes;

	return $text;
}

And the code on our end might look something like this:

function add_mathjax_shortcode() {
	$latex = preg_replace( array( '#<br\s*/?>#i', '#</?p>#i' ), ' ', $latex );

	$latex = str_replace(
		array( '&lt;', '&gt;', '&quot;', '&#8220;', '&#8221;', '&#039;', '&#8125;', '&#8127;', '&#8217;', '&#038;', '&amp;', "\n", "\r", "\xa0", '&#8211;' ),
		array( '<',    '>',    '"',      '``',       "''",     "'",      "'",       "'",       "'",       '&',      '&',     ' ',  ' ',  ' ',    '-' ),
		$latex
	);

	return '[latex]' . $latex . '[/latex]';
}
add_filter( 'pb_add_latex_shortcode', '\Candela\Utility\Latex\add_mathjax_shortcode' );
Contributor

monkecheese commented Oct 4, 2017

@connerbw

The problem is in the creation of the shortcode here. The config script that we load here is looking for [latex] ... [/latex] on the page, and instead it's finding the <img src='$url' alt='$alt' title='$alt' class='latex' /> being built by the shortCode function.

Another possibly better solution might be to add a new filter that allows us to return our own shortcode (bypassing the shortCode function) in favor of our own that allows mathjax to render properly.

Example: (pb_add_latex_shortcode)

function doThisShortcode( $text ) {
	$current_shortcodes = $GLOBALS['shortcode_tags'];
	remove_all_shortcodes();

	add_shortcode(
		'latex',
		apply_filters( 'pb_add_latex_shortcode', [ $this, 'shortCode' ] ) );

	$text = do_shortcode( $text );

	$GLOBALS['shortcode_tags'] = $current_shortcodes;

	return $text;
}

And the code on our end might look something like this:

function add_mathjax_shortcode() {
	$latex = preg_replace( array( '#<br\s*/?>#i', '#</?p>#i' ), ' ', $latex );

	$latex = str_replace(
		array( '&lt;', '&gt;', '&quot;', '&#8220;', '&#8221;', '&#039;', '&#8125;', '&#8127;', '&#8217;', '&#038;', '&amp;', "\n", "\r", "\xa0", '&#8211;' ),
		array( '<',    '>',    '"',      '``',       "''",     "'",      "'",       "'",       "'",       '&',      '&',     ' ',  ' ',  ' ',    '-' ),
		$latex
	);

	return '[latex]' . $latex . '[/latex]';
}
add_filter( 'pb_add_latex_shortcode', '\Candela\Utility\Latex\add_mathjax_shortcode' );
@connerbw

This comment has been minimized.

Show comment
Hide comment
@connerbw

connerbw Oct 4, 2017

Member

What's the simplest solution? Remove the line? I'd rather do that, say with a config variable, than add a new filter that may be deprecated soon (math is on our road map). Is that OK?

Member

connerbw commented Oct 4, 2017

What's the simplest solution? Remove the line? I'd rather do that, say with a config variable, than add a new filter that may be deprecated soon (math is on our road map). Is that OK?

@connerbw

This comment has been minimized.

Show comment
Hide comment
@connerbw
Member

connerbw commented Oct 4, 2017

@monkecheese

This comment has been minimized.

Show comment
Hide comment
@monkecheese

monkecheese Oct 4, 2017

Contributor

Both options are simple for me to implement. BUT I lean toward the latter solution because, moving forward, if mathjax is something you guys are hoping to add to core, then doing the pb_add_latex_shortcode filter hook might be smarter because it's something you might end up using down the line when you circle back on math rendering (per your roadmap). If you find that it isn't helpful and you find a better solution for adding mathjax to core, the filter could be deprecated. Either way though, I'm okay with both options.

Contributor

monkecheese commented Oct 4, 2017

Both options are simple for me to implement. BUT I lean toward the latter solution because, moving forward, if mathjax is something you guys are hoping to add to core, then doing the pb_add_latex_shortcode filter hook might be smarter because it's something you might end up using down the line when you circle back on math rendering (per your roadmap). If you find that it isn't helpful and you find a better solution for adding mathjax to core, the filter could be deprecated. Either way though, I'm okay with both options.

@monkecheese

This comment has been minimized.

Show comment
Hide comment
@monkecheese

monkecheese Oct 4, 2017

Contributor

On second thought, I can appreciate why you wouldn't want to add a new filter that is more or less likely to be deprecated later. The safer bet might actually be to do the first suggestion for now.

Contributor

monkecheese commented Oct 4, 2017

On second thought, I can appreciate why you wouldn't want to add a new filter that is more or less likely to be deprecated later. The safer bet might actually be to do the first suggestion for now.

@greatislander

This comment has been minimized.

Show comment
Hide comment
@greatislander

greatislander Oct 4, 2017

Member

If you wouldn't mind submitting a PR I'd be happy to merge in the AM. Thanks for talking this through with us!

Member

greatislander commented Oct 4, 2017

If you wouldn't mind submitting a PR I'd be happy to merge in the AM. Thanks for talking this through with us!

@monkecheese

This comment has been minimized.

Show comment
Hide comment
@monkecheese

monkecheese Oct 4, 2017

Contributor

PR submitted: #959

Contributor

monkecheese commented Oct 4, 2017

PR submitted: #959

@monkecheese

This comment has been minimized.

Show comment
Hide comment
@monkecheese

monkecheese Oct 5, 2017

Contributor

I just saw the comment about using a config variable instead of checking the pb_enqueue_latex_scripts hook (not sure how I missed that). If that's preferred, I'd be happy to update the PR. Cheers!

Contributor

monkecheese commented Oct 5, 2017

I just saw the comment about using a config variable instead of checking the pb_enqueue_latex_scripts hook (not sure how I missed that). If that's preferred, I'd be happy to update the PR. Cheers!

@greatislander

This comment has been minimized.

Show comment
Hide comment
@greatislander

greatislander Oct 5, 2017

Member

All good, just merged #959.

Member

greatislander commented Oct 5, 2017

All good, just merged #959.

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