#29557 closed defect (bug) (fixed)
PHP ≤ 5.4.8 Crashes on '[' Character in Posts
Reported by: | MrBobDobolina | Owned by: | nacin |
---|---|---|---|
Milestone: | 4.0.1 | Priority: | highest omg bbq |
Severity: | blocker | Version: | 4.0 |
Component: | Formatting | Keywords: | wptexturize has-patch commit fixed-major |
Focuses: | Cc: |
Description
After updating to WP 4, one of my longer blog posts is failing to work. But it's only the one post, everything else works fine.
The post page doesn’t render anything, it doesn’t serve anything, all you get in inspector is a blank HTML document.
The post can still be edited in the admin interface, but neither previewing or posting the article allows it to be seen.
A Wordpress 3.9.2 installation with the Twenty Twelve theme works as intended. There are no errors in the Apache or MySQL logs.
The post is 1030 lines long with paragraphs and css declarations and images on almost every line. (I have included it for testing purposes.) If I shorten it to ~793 lines it works as intended. (I have tried shortening it from both ends, with the same result. It does not appear to be related to the specific post content.)
Attachments (24)
Change History (155)
#1
@
10 years ago
- Component changed from Posts, Post Types to Formatting
- Keywords wptexturize added
- Severity changed from normal to major
Neato. php-cgi.exe crashes on my Windows server when I try to run the new HTML regexp for wptexturize() against the attachment.
#2
@
10 years ago
- Summary changed from No Data Received - long post rendering issue to PHP Crash on Large Block of HTML
#3
@
10 years ago
- Summary changed from PHP Crash on Large Block of HTML to No Data Received - long post rendering issue
Try adding this to your wp-config file:
if (ini_get('pcre.backtrack_limit') < 1000000) { ini_set('pcre.backtrack_limit', 1000000); }
#4
follow-up:
↓ 5
@
10 years ago
- Summary changed from No Data Received - long post rendering issue to PHP Crash on Large Block of HTML
MrBobDobolina, can you confirm you are running version 4.0 and not one of the beta versions? I was able to reproduce the crash with a slightly older version that we fixed at [28852]. I'm not sure if this is the same problem.
#5
in reply to:
↑ 4
@
10 years ago
Replying to miqrogroove:
MrBobDobolina, can you confirm you are running version 4.0 and not one of the beta versions? I was able to reproduce the crash with a slightly older version that we fixed at [28852]. I'm not sure if this is the same problem.
Can confirm that I am running 4.0. Not a beta.
#6
follow-up:
↓ 7
@
10 years ago
#7
in reply to:
↑ 6
@
10 years ago
Replying to SergeyBiryukov:
Confirmed on 4.0. Fails on this line in
wptexturize()
.
Increasing
pcre.backtrack_limit
to1000000
didn't help.
Increasing will actually make this worse. I can get a crash on the beta version when the limit is greater than 50000. We thought this was fixed, and I'm getting no errors in the 4.0 version so far. Still looking.
#8
@
10 years ago
Getting a backtrack limit on the order of 16000 for the attached file, and recursion limit less than 10.
Sergey, can you confirm you do not get a crash with the backtrack limit set to 10000? This would cause the preg_split() statement to fail silently.
#9
@
10 years ago
Half of the backtracking can be eliminated by changing '[^\[\]<>]'
to '[^\[\]<>]++'
. The remainder is being caused by the nested alternation following that part. Still looking.
#10
follow-up:
↓ 13
@
10 years ago
On PHP 5.2.17 (PCRE 8.02 2010-03-19), I get a crash with any backtrack limit value larger than 709.
On PHP 5.4.29 (PCRE 8.32 2012-11-30), I don't get a crash even with a much larger value, 1,000,000,000.
#11
@
10 years ago
MrBobDobolina may have found the Achilles heel of this regexp. There is only one square brace in the entire post. Remove that one character and try to reproduce the crash again.
#12
@
10 years ago
We may have a poor solution for #28564. What is happening internally is PCRE is looking at every possible combination of HTML and shortcode nesting before checking if there is actually a closing brace for the shortcode. This is a wonderful argument for disallowing HTML nesting within the shortcode feature.
#13
in reply to:
↑ 10
@
10 years ago
Replying to SergeyBiryukov:
On PHP 5.2.17 (PCRE 8.02 2010-03-19), I get a crash with any backtrack limit value larger than 709.
On PHP 5.4.29 (PCRE 8.32 2012-11-30), I don't get a crash even with a much larger value, 1,000,000,000.
My hunch is this is the underlying issue. PHP 5.3.7 fixes a few regex related things, and -- best I'm aware -- #8553 et al would not occur when running WP on top of that PHP version or later.
#15
@
10 years ago
I'll get started on a patch to import the list of registered shortcodes into wptexturize(). This may be the only way to eliminate backtracking without changing the behavior of the regexp.
#16
@
10 years ago
In miqro-29557.patch:
- Expand the wptexturize regexp to include the list of registered shortcodes.
- This will cause escaped shortcodes to become texturized if not registered.
- Registered shortcodes will not be texturized, even when escaped.
- Remove all tests involving unregistered shortcodes.
- Benchmark result: No measurable impact on performance.
For more consistency, we could remove the check on escaped shortcodes altogether and just always texturize them. They are escaped, so they have no special meaning, and they can always be wrapped in <code>
blocks just like any other text. I only hesitate to do this because it would represent an additional change in behavior.
This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. View the logs.
10 years ago
#20
@
10 years ago
There's more work to do on those unit tests to make sure the patch is legit. I'll get a second draft up here tomorrow.
#21
@
10 years ago
miqro-29557.3.patch attached. Made sure it was working as described above. With the shortcode name matching, it is also necessary to allow for the forward slash before the name, and then the shortcode contents after the name become optional.
#23
@
10 years ago
MrBobDobolina, the attached file miqro-29557.4.patch should fix the problem if you want to hack the formatting.php file on your website. Otherwise, the workaround appears to be removing that one [
character from the blog post until the fixed version is released. Let us know if that helps.
#24
@
10 years ago
Thanks miqrogroove
I took out the loose parenthetical the other day when I saw it might cause the problem. It seems to be working now.
You guys rocks!
#25
follow-up:
↓ 27
@
10 years ago
Hi guys. I've faced with the same issue on the line
$textarr = preg_split( $regex, $text, -1, PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY );
I attached a file Content1.txt. This is a dump of the $text variable just before preg_split. OS Windows 7 Pro, PHP version is 5.3.13, PCRE Library Version 8.12 2011-01-15, pcre.backtrack_limit = 1000000.
I applied the patch miqro-29557.4.patch, it helped to pass preg_split, but it crashes somewhere after. I'll try to locate the line. Could you please take a look as well. Thanks.
#27
in reply to:
↑ 25
;
follow-up:
↓ 28
@
10 years ago
Replying to ArsenyKovalchuk:
Hi guys. I've faced with the same issue on the line
$textarr = preg_split( $regex, $text, -1, PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY );I attached a file Content1.txt. This is a dump of the $text variable just before preg_split. OS Windows 7 Pro, PHP version is 5.3.13, PCRE Library Version 8.12 2011-01-15, pcre.backtrack_limit = 1000000.
I applied the patch miqro-29557.4.patch, it helped to pass preg_split, but it crashes somewhere after. I'll try to locate the line. Could you please take a look as well. Thanks.
Now it crashes in the loop on the first line of the text (line 250 of the pathced code)
} elseif ( '[' === $first && 1 === preg_match( '/^\[\[?\/?' . $tagregexp . '(?:[^\[\]<>]|<[^>]+>)*+\]\]?$/', $curl ) ) {
I get following regexp code which crashes
preg_match('/^\[\[?\/?(?:embed|wp_caption|caption|gallery|playlist|audio|video|layerslider|adrotate|acf|go_pricing|go_pricing_youtube|go_pricing_vimeo|go_pricing_screenr|go_pricing_dailymotion|go_pricing_metacafe|go_pricing_html5_video|go_pricing_soundcloud|go_pricing_mixcloud|go_pricing_beatport|go_pricing_audio|go_pricing_map|go_pricing_custom_iframe|free_adv_form|ahm\-pricing\-table|rev_slider|page_polls|poll|ratings|p2p_connected|p2p_related|contact\-form\-7|contact\-form|vc_container_anchor|dt_cell|dt_box|dt_gap|dt_divider|dt_stripe|dt_fancy_image|dt_list_item|dt_list|dt_button|dt_tooltip|dt_highlight|dt_code|dt_code_final|dt_tab|dt_tabs|dt_item|dt_accordion|dt_toggle|dt_quote|dt_call_to_action|dt_teaser|dt_banner|dt_benefits|dt_benefit|dt_progress_bars|dt_progress_bar|dt_contact_form|dt_social_icons|dt_social_icon|dt_map|dt_blog_posts_small|dt_blog_posts|dt_portfolio|dt_portfolio_jgrid|dt_portfolio_slider|dt_small_photos|dt_slideshow|dt_team|dt_testimonials|dt_logos|dt_text|dt_vc_list_item|dt_vc_list|dt_benefits_vc|dt_fancy_video_vc|dt_fancy_title|dt_fancy_separator|types|vc_row|vc_row_inner|vc_column|vc_column_text|vc_separator|vc_text_separator|vc_message|vc_facebook|vc_tweetmeme|vc_googleplus|vc_pinterest|vc_toggle|vc_single_image|vc_tabs|vc_tour|vc_tab|vc_accordion|vc_accordion_tab|vc_carousel|vc_posts_slider|vc_widget_sidebar|vc_button2|vc_cta_button2|vc_video|vc_gmaps|vc_raw_html|vc_raw_js|vc_flickr|vc_progress_bar|vc_pie|layerslider_vc|rev_slider_vc|vc_column_inner)(?![\w-])(?:[^\[\]<>]|<[^>]+>)*+\]\]?$/','[vc_row margin_top="35" margin_bottom="45" padding_left="40" padding_right="40" bg_position="top" bg_repeat="no-repeat" bg_cover="false" bg_attachment="false" padding_top="40" padding_bottom="40" parallax_speed="0.1"][vc_column width="3/4"][dt_benefits_vc columns="4" style="1" dividers="true" image_background="true" target_blank="false" header_size="h5" content_size="normal" number="4" orderby="date" order="desc" animation="scale" category="no-captions,no-buttons" image_background_color="#ffffff"]')
#28
in reply to:
↑ 27
;
follow-up:
↓ 30
@
10 years ago
Replying to ArsenyKovalchuk:
I get following regexp code which crashes
preg_match('/^\[\[?\/?(?:embed|wp_caption|caption|gallery|playlist|audio|video|layerslider|adrotate|acf|go_pricing|go_pricing_youtube|go_pricing_vimeo|go_pricing_screenr|go_pricing_dailymotion|go_pricing_metacafe|go_pricing_html5_video|go_pricing_soundcloud|go_pricing_mixcloud|go_pricing_beatport|go_pricing_audio|go_pricing_map|go_pricing_custom_iframe|free_adv_form|ahm\-pricing\-table|rev_slider|page_polls|poll|ratings|p2p_connected|p2p_related|contact\-form\-7|contact\-form|vc_container_anchor|dt_cell|dt_box|dt_gap|dt_divider|dt_stripe|dt_fancy_image|dt_list_item|dt_list|dt_button|dt_tooltip|dt_highlight|dt_code|dt_code_final|dt_tab|dt_tabs|dt_item|dt_accordion|dt_toggle|dt_quote|dt_call_to_action|dt_teaser|dt_banner|dt_benefits|dt_benefit|dt_progress_bars|dt_progress_bar|dt_contact_form|dt_social_icons|dt_social_icon|dt_map|dt_blog_posts_small|dt_blog_posts|dt_portfolio|dt_portfolio_jgrid|dt_portfolio_slider|dt_small_photos|dt_slideshow|dt_team|dt_testimonials|dt_logos|dt_text|dt_vc_list_item|dt_vc_list|dt_benefits_vc|dt_fancy_video_vc|dt_fancy_title|dt_fancy_separator|types|vc_row|vc_row_inner|vc_column|vc_column_text|vc_separator|vc_text_separator|vc_message|vc_facebook|vc_tweetmeme|vc_googleplus|vc_pinterest|vc_toggle|vc_single_image|vc_tabs|vc_tour|vc_tab|vc_accordion|vc_accordion_tab|vc_carousel|vc_posts_slider|vc_widget_sidebar|vc_button2|vc_cta_button2|vc_video|vc_gmaps|vc_raw_html|vc_raw_js|vc_flickr|vc_progress_bar|vc_pie|layerslider_vc|rev_slider_vc|vc_column_inner)(?![\w-])(?:[^\[\]<>]|<[^>]+>)*+\]\]?$/','[vc_row margin_top="35" margin_bottom="45" padding_left="40" padding_right="40" bg_position="top" bg_repeat="no-repeat" bg_cover="false" bg_attachment="false" padding_top="40" padding_bottom="40" parallax_speed="0.1"][vc_column width="3/4"][dt_benefits_vc columns="4" style="1" dividers="true" image_background="true" target_blank="false" header_size="h5" content_size="normal" number="4" orderby="date" order="desc" animation="scale" category="no-captions,no-buttons" image_background_color="#ffffff"]')
So that one command crashes your server if you put it in a php file? I want to be specific, because the backtrack count for that command is only on the order of 400. This should be less than the pattern before the loop.
#29
@
10 years ago
It looks like my patch broke part of the shortcodes pattern. I'll look into getting you a better patch and then let's see what happens after that.
#30
in reply to:
↑ 28
@
10 years ago
Replying to miqrogroove:
Replying to ArsenyKovalchuk:
I get following regexp code which crashes
preg_match('/^\[\[?\/?(?:embed|wp_caption|caption|gallery|playlist|audio|video|layerslider|adrotate|acf|go_pricing|go_pricing_youtube|go_pricing_vimeo|go_pricing_screenr|go_pricing_dailymotion|go_pricing_metacafe|go_pricing_html5_video|go_pricing_soundcloud|go_pricing_mixcloud|go_pricing_beatport|go_pricing_audio|go_pricing_map|go_pricing_custom_iframe|free_adv_form|ahm\-pricing\-table|rev_slider|page_polls|poll|ratings|p2p_connected|p2p_related|contact\-form\-7|contact\-form|vc_container_anchor|dt_cell|dt_box|dt_gap|dt_divider|dt_stripe|dt_fancy_image|dt_list_item|dt_list|dt_button|dt_tooltip|dt_highlight|dt_code|dt_code_final|dt_tab|dt_tabs|dt_item|dt_accordion|dt_toggle|dt_quote|dt_call_to_action|dt_teaser|dt_banner|dt_benefits|dt_benefit|dt_progress_bars|dt_progress_bar|dt_contact_form|dt_social_icons|dt_social_icon|dt_map|dt_blog_posts_small|dt_blog_posts|dt_portfolio|dt_portfolio_jgrid|dt_portfolio_slider|dt_small_photos|dt_slideshow|dt_team|dt_testimonials|dt_logos|dt_text|dt_vc_list_item|dt_vc_list|dt_benefits_vc|dt_fancy_video_vc|dt_fancy_title|dt_fancy_separator|types|vc_row|vc_row_inner|vc_column|vc_column_text|vc_separator|vc_text_separator|vc_message|vc_facebook|vc_tweetmeme|vc_googleplus|vc_pinterest|vc_toggle|vc_single_image|vc_tabs|vc_tour|vc_tab|vc_accordion|vc_accordion_tab|vc_carousel|vc_posts_slider|vc_widget_sidebar|vc_button2|vc_cta_button2|vc_video|vc_gmaps|vc_raw_html|vc_raw_js|vc_flickr|vc_progress_bar|vc_pie|layerslider_vc|rev_slider_vc|vc_column_inner)(?![\w-])(?:[^\[\]<>]|<[^>]+>)*+\]\]?$/','[vc_row margin_top="35" margin_bottom="45" padding_left="40" padding_right="40" bg_position="top" bg_repeat="no-repeat" bg_cover="false" bg_attachment="false" padding_top="40" padding_bottom="40" parallax_speed="0.1"][vc_column width="3/4"][dt_benefits_vc columns="4" style="1" dividers="true" image_background="true" target_blank="false" header_size="h5" content_size="normal" number="4" orderby="date" order="desc" animation="scale" category="no-captions,no-buttons" image_background_color="#ffffff"]')So that one command crashes your server if you put it in a php file? I want to be specific, because the trackback count for that command is only on the order of 400. This should be less than the pattern before the loop.
Hi, as you may expect I got that command by logging following inside the loop
$str = '/^\[\[?\/?' . $tagregexp . '(?:[^\[\]<>]|<[^>]+>)*+\]\]?$/'; error_log("preg_match('$str','$curl')");
And, yes, the code crashes the server if I put it in a separate file.
#31
@
10 years ago
One thing I'm not understanding is why there are 3 different shortcodes in your value for $curl there.
#32
follow-up:
↓ 33
@
10 years ago
ArsenyKovalchuk, if you could message me through IRC or gmail or my website, I think we could look at more details.
#33
in reply to:
↑ 32
;
follow-ups:
↓ 34
↓ 35
@
10 years ago
Replying to miqrogroove:
ArsenyKovalchuk, if you could message me through IRC or gmail or my website, I think we could look at more details.
It would be more convenient to contact via gmail, didn't find your email though. Mine is ars.kov@… just send me a message.
I attached a dump of the $testarr just after preg_split. The $text passed to preg_split is in the Content1.txt
#34
in reply to:
↑ 33
@
10 years ago
Replying to ArsenyKovalchuk:
Replying to miqrogroove:
ArsenyKovalchuk, if you could message me through IRC or gmail or my website, I think we could look at more details.
It would be more convenient to contact via gmail, didn't find your email though. Mine is ars.kov@… just send me a message.
I attached a dump of the $testarr just after preg_split. The $text passed to preg_split is in the Content1.txt
Also I attached dump of $shortcode_tags keys inside the function call.
#35
in reply to:
↑ 33
;
follow-up:
↓ 36
@
10 years ago
Replying to ArsenyKovalchuk:
I attached a dump of the $testarr just after preg_split. The $text passed to preg_split is in the Content1.txt
Yeah I'm not understanding how we arrived at that result. First I'd like to see the formatting.php file, which you could send on gmail or attach here. I need to see that the patch was applied correctly before we look for other bugs.
#36
in reply to:
↑ 35
@
10 years ago
Replying to miqrogroove:
Replying to ArsenyKovalchuk:
I attached a dump of the $testarr just after preg_split. The $text passed to preg_split is in the Content1.txt
Yeah I'm not understanding how we arrived at that result. First I'd like to see the formatting.php file, which you could send on gmail or attach here. I need to see that the patch was applied correctly before we look for other bugs.
miqro-29557.5.patch works for me. I've also noticed that WP renders pages much faster now. I also had another couple of pages that crashed the server, but they also render ok with the patch.
#38
@
10 years ago
Added some much-needed explanations at http://codex.wordpress.org/Shortcode_API#Formal_Syntax
#41
@
10 years ago
In working on a project for the wp.org Code Reference and I just across this problem -- not being able pass things like less- and greater-than characters via shortcode attributes.
I tested miqro-29557.5.patch and it solves the issues I'm seeing with passing values like 'WP_Query->have_posts()', etc.
+1 for miqro-29557.5.patch.
#42
@
10 years ago
Updated summary of miqro-29557.5.patch:
- Expand the wptexturize regexp to include the list of registered shortcodes.
- Avoid backtracking after
[
chars by not filtering params in registered shortcodes. - This will cause escaped shortcodes and their params to become texturized if not registered.
- Registered shortcode params will never be texturized, even when escaped.
- Move all tests involving unregistered shortcodes to a new and improved unit.
- Update one test involving HTML within shortcode params.
Tests for dangling angle braces within shortcode params have not been added. This feature is not even documented AFAIK. In the Codex, HTML is only mentioned as allowed in between shortcode tags.
Decided not to remove the check on escaped shortcodes. There are too many potential corner cases involving the different regexps for texturize and shortcodes. Instead, I simply nested the if statement to help speed up shortcode processing.
#43
@
10 years ago
- Priority changed from normal to highest omg bbq
- Severity changed from major to blocker
#45
follow-up:
↓ 47
@
10 years ago
Discovered in follow up to #29649 that PHP versions 5.3.19+ and 5.4.9+ are not affected by this bug.
Currently looking for an old server I could abuse.
#46
@
10 years ago
- Summary changed from PHP Crash on Large Block of HTML to PHP ≤ 5.4.8 Crashes on '[' Character in Posts
#47
in reply to:
↑ 45
@
10 years ago
Hi miqrogroove,
I'm using this XAMPP (PHP 5.4.7) bundle to test:
Replying to miqrogroove:
Discovered in follow up to #29649 that PHP versions 5.3.19+ and 5.4.9+ are not affected by this bug.
Currently looking for an old server I could abuse.
#48
follow-up:
↓ 49
@
10 years ago
Results of testing PHP 5.2.4 and PHP 5.2.13.
Using 4.0-beta code.
Test case caused about 60000 backtracks in 5.2.4. It did not crash, but note this would be close to the default backtrack limit for that version.
v5.2.13 reached about 24700 backtracks and then crashed.
Using 4.0.0 code.
Test case caused about 30000 backtracks in 5.2.4.
v5.2.13 reached about 24700 backtracks and then crashed.
Note kovshenin's comment appears to be incorrect https://core.trac.wordpress.org/ticket/12690#comment:37 What we thought was a solution only decreased the backtrack count by half, meaning a post twice as long would still crash PHP.
Using 4.0.0 code with second extra possessive as discussed.
Test case caused about 8000 backtracks in 5.2.4 as well as 5.2.13.
Using the .4 patch code.
Test case caused about 10 backtracks for unregistered shortcodes and 20 backtracks for registered shortcodes.
Using the .5 patch code.
Test case caused about 10 backtracks for unregistered shortcodes and 10 backtracks for registered shortcodes.
I can't yet explain why some people seem to be having better luck with the .5 patch than the .4 patch. My comparison of the two is inconclusive, but I'm comfortable using the .5 patch based on the results so far.
#49
in reply to:
↑ 48
@
10 years ago
Replying to miqrogroove:
I can't yet explain why some people seem to be having better luck with the .5 patch than the .4 patch. My comparison of the two is inconclusive, but I'm comfortable using the .5 patch based on the results so far.
Found the answer to that now. The .4 patch did not include the second possessive quantifier, and without it the number of backtracks skyrockets to the order of 1000 in my test case. So if we need an alternative patch for some reason, that would be a possible tweak.
#52
@
10 years ago
Of note, this bug has made posting from my wp-admin impossible, preventing login to the site. Testing @miqrogroove's patches had mixed results, both .4 and .5 patch fix the issue on a local instance I have running but the issue remains when I test on staging.
#53
@
10 years ago
Hi brianfeister, this bug is not related to the wp-admin system and so the patch would not resolve login problems.
#54
@
10 years ago
I can't attend the developer meeting today. I am in favor of committing miqro-29557.5.patch as soon as possible for release with 4.0.1. I can answer any questions about this tonight or tomorrow.
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
10 years ago
#57
@
10 years ago
- Keywords commit fixed-major added
I'd like this to be reviewed by a few others before it gets merged to the branch. In particular, I'm seeking reviews from kovshenin and mdawaffe.
#58
follow-up:
↓ 59
@
10 years ago
From IRC:
I think "Shortcodes do not contain other shortcodes." is wrong
To be completely precise, the ']' character is never allowed inside of a shortcode tag, so by extrapolation there is no way to put a raw tag within a tag. Between tags, almost anything is allowed.
If you'd like me to research the technical implications of allowing the '[' character inside of a shortcode, I could entertain that thought. But that sort of thing could take a couple weeks given my schedule.
#59
in reply to:
↑ 58
;
follow-up:
↓ 60
@
10 years ago
Replying to miqrogroove:
If you'd like me to research the technical implications of allowing the '[' character inside of a shortcode, I could entertain that thought.
Up to WordPress 3.9, left square brackets in shortcode parameters were accepted without a problem.
Right square brackets could not be used; more often than not they simply ended the shortcode.
IMHO: There's probably little point in re-enabling support for left square brackets without allowing right square brackets as well.
BTW: Nice codex updates.
#60
in reply to:
↑ 59
@
10 years ago
Replying to bobbingwide:
Up to WordPress 3.9, left square brackets in shortcode parameters were accepted without a problem.
I think some combinations of HTML and left brackets might cause problems in 3.9. Much more testing would be needed to know for sure.
#61
follow-up:
↓ 62
@
10 years ago
r29748 creates issues with plugins using shortcodes wrapped in another shortcode, e.g. Jetpack's Contact Form shortcode (multiple contact-field
shortcodes wrapped into a contact-form
shortcode).
#62
in reply to:
↑ 61
@
10 years ago
Replying to jeherve:
r29748 creates issues with plugins using shortcodes wrapped in another shortcode, e.g. Jetpack's Contact Form shortcode (multiple
contact-field
shortcodes wrapped into acontact-form
shortcode).
There are no issues with the example code on the Jetpack site
[contact-form submit_button_text='YOUR CUSTOM SUBMIT BUTTON TEXT HERE'][contact-field label='Name' type='name' required='1'/][contact-field label='Email' type='email' required='1'/][contact-field label='Website' type='url'/][contact-field label='Comment' type='textarea' required='1'/][/contact-form]
#63
@
10 years ago
There does appear to be a problem with the plugin itself, which does not register the contact-field shortcode. In that case, the contact-field shortcode is always invalid.
#64
@
10 years ago
The Jetpack plugin also fails to add the contact-form shortcode to the no_texturize list. So, it will be up to the plugin developers to fix those problems one way or the other.
#65
@
10 years ago
Added more explanations at http://codex.wordpress.org/Shortcode_API#Nested_Shortcodes
Although the patch isn't yet official in 4.0.1, it would be good/best practice to expect unregistered shortcodes to become texturized.
This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. View the logs.
10 years ago
#67
@
10 years ago
Per IRC discussion, we will not be able to resolve the crash by breaking non-registered shortcodes. :(
In miqro-29557.6.patch:
- Revert parts of miqro-29557.5.patch.
- Revert [28773] and remove 'attribute filter' from [28727].
- Make the shortcode pattern quantifier possessive to avoid backtracks.
- Security regression: HTML filter avoidance may be possible again per #12690.
- Does not break unregistered shortcodes.
- Does not break HTML comments or performance gains from 4.0.
- Tested PHP 5.2.4, 5.2.13, 5.4.32, and 5.5.8.
- No crashes or large backtrack counts found in any version.
- Unit tests updated to show new outputs.
#68
@
10 years ago
Extra thoughts on the security impact:
From a larger perspective, neither patch would resolve potential HTML corruption. With miqro-29557.5.patch a user who is allowed the necessary contexts could do this:
[caption - Is it wise to <a title="allow user content ] here? hmm"> maybe </a> ]
Now assume the context does not support shortcodes, but is texturized. Currently wptexturize() is unaware of the shortcode filter status and will always avoid shortcodes. If shortcodes are in fact disabled, then wptexturize() has avoided the wrong code, resulting in texturized HTML elements.
I'm mentioning this here for two reasons: It is a known bug, and I think miqro-29557.6.patch does not represent a significant regression as I did earlier.
#69
@
10 years ago
Found a small refinement and added more tests based on ideas above.
In miqro-29557.7.patch:
- Revert parts of miqro-29557.5.patch.
- Revert parts of [28773] and [28727].
- Do not crash PHP. Make the pattern quantifier possessive to avoid backtracks.
- Do not break unregistered shortcodes, e.g. [hello attr="value"].
- Do not break HTML in shortcode attributes, e.g. [hello attr="<"].
- Do not match for shortcodes when there is extra whitespace, e.g. [ hello ].
- Add unit tests to show #12690 was not fully resolved.
- Tested PHP 5.2.4, 5.2.13, 5.4.32, and 5.5.8.
#70
@
10 years ago
- Keywords has-patch commit fixed-major removed
Need to test performance with long HTML comments. I'm not seeing any crashes but getting paranoid about this whole backtracking idea.
#71
@
10 years ago
- Keywords has-patch added
I experimented with several more solutions and this one seems to be the most optimal so far.
In miqro-29557.8.patch:
- Revert parts of [28773] and [28727] and [29748].
- Do not crash PHP. Make the shortcode quantifier possessive to avoid backtracks.
- Reduce backtracking in long HTML comments by 100x.
- Do not ignore unclosed HTML comments.
- Do not break unregistered shortcodes, e.g. [hello attr="value"].
- Do not break HTML in shortcode attributes, e.g. [hello attr="<"].
- Do not match for shortcodes when there is extra whitespace, e.g. [ hello ].
- Add unit tests to show #12690 was not fully resolved.
- Tested PHP 5.2.4, 5.2.13, 5.4.32, and 5.5.8.
#72
@
10 years ago
The comment parsing could be simplified. Gecko, Webkit and Trident all (apparently) use this:
(<!--.+?-->|<!--+>)
even in exotic doctypes like xml and 4.01 strict. Despite the W3C, \s*>
is not allowed in 4.01 strict. And embedded --
are ignored, since sgml legacy is not as important as easily commenting out html (or xml). There's probably some scenario I missed, but this regex is much simpler than the one in the latest patch. I separated the clauses completely with |
to avoid any issues combining the greedy and non-greedy parts.
Test strings:
Shows 'ab':
a<!-->b a<!--->b a<!---->b a<!----->b a<!-- c --->b a<!-- c -- d -->b
Does not show 'ab':
a<!-- c -- >b<!-- close --> a<!-- >b<!-- close --> a<!---- >b<!-- close --> a<!-- <!-- c --> -->b<!-- close -->
The regex would be a change from current WP because it disallows whitespace before closing >
. But that's how the browser renders it anyway.
(Trident junk like <!--[if IE ]>
doesn't matter in this context of course.)
#73
@
10 years ago
Can't use .+?
due to the backtracking limitations but I should be able to drop the \s*
Some of those test strings should not be valid anyway. <!--->
does not seem to have any useful purpose and is expressly forbidden in HTML 5.
#74
@
10 years ago
Hi kitchin, thank you for the feedback. Check out miqro-29557.10.patch which should match the browser behavior and give a lower backtrack count than the suggested pattern.
#76
follow-up:
↓ 103
@
10 years ago
Miqrogroove, I did test patch 10 and all was fine. I compared it to an algorithm that first did the shortcode regex and then used strpos() to do the tag/comment parsing. The only tests that failed were the expected ones by doing it that way: shortcode-like text within html tags. Also tested it the other way (tags then shortcodes) and got expected failures. So you have the right tests as far as I see.
I thought skipping internal --
in comments would simplify things, but your algorithm does skip them and does not backtrack. Like you said!
#77
@
10 years ago
[29781] has some whitespace issues - should use spaces instead of tabs for alignment, and there's a little bit of trailing in there.
#78
@
10 years ago
Whitespace attached.
Codex page expanded and updated.
#79
@
10 years ago
#29810 has been spun off as a separate issue. I'm certain now there will be no way to fix that particular problem until 4.1 or 4.2.
#80
follow-up:
↓ 81
@
10 years ago
Looking at the patches here and wondering: would it be better to do a double preg_split()? Something like:
if ( strpos( $text, '[' ) !== false && strpos( $text, ']' ) !== false ) { $has_shortcodes = true; $textarr = preg_split( '/generic shortcode regexp/', $text, ... ); } else { $textarr = array( $text ); } foreach ( $textarr as &$part ) { if ( ! $has_shortcodes || not a shortcode ) { $split = preg_split( '/<.[^>]*>/', $part, etc... ); foreach ( $split as &$chunk ) { // Process the text and tags } } }
This will make the regexp simpler, remove a lot of backtracking, prevent cases like in comment 68, and bypass tags in shortcode attributes.
#81
in reply to:
↑ 80
@
10 years ago
Replying to azaozz:
This will make the regexp simpler, remove a lot of backtracking, prevent cases like in comment 68, and bypass tags in shortcode attributes.
Actually, I think it would make most of those problems worse. Let's chat on IRC.
#82
@
10 years ago
IRC Summary:
- We can try some more strpos() tricks in patches for #29810.
- A few dry runs showed a 10% speed boost in posts that don't have any shortcodes.
- Adding preg_split() inside a loop does not seem like a good direction for performance.
- There are no new concerns for 4.0.1 at this time.
#83
follow-up:
↓ 84
@
10 years ago
azaozz, I tested a patch that does essentially that, and it fails that following tests:
14.Test.. '<br [gallery ...] ... />' 14.Actual '<br [gallery ...] … />' 14.Expect '<br [gallery ...] ... />' 24.Test.. '<br [[gallery ...]] ... />' 24.Actual '<br [[gallery ...]] … />' 24.Expect '<br [[gallery ...]] ... />' 39.Test.. '<!-- <br /> [gallery] ... -->' 39.Actual '<!-- <br /> [gallery] … –>' 39.Expect '<!-- <br /> [gallery] ... -->' 45.Test.. '[ regex catches this <a href="[quote]">here</a> ]' 45.Actual '[ regex catches this <a href=”[quote]“>here</a> ]' 45.Expect '[ regex catches this <a href="[quote]">here</a> ]'
My code uses better tag logic in the second part of your suggestion to catch comments (all done with strpos()), but it's the same idea. The reverse idea, tags first then shortcodes, also fails tests.
Miqro's patch is ideal for the current tests, with one caveat: the tag regex is executed twice for each match (once in the implicit preg_split
loop and once in the foreach
loop). To avoid running the regex twice I have some code that saves data on each part of the split match, but it's execution expensive.
#84
in reply to:
↑ 83
;
follow-up:
↓ 102
@
10 years ago
Replying to kitchin:
Miqro's patch is ideal for the current tests, with one caveat: the tag regex is executed twice for each match (once in the implicit
preg_split
loop and once in theforeach
loop).
preg_match_all() might be able to structure the results better than preg_split(). I'm not sure what the speed differences would be. Also, I'm not sure there would be any elegant way to capture the parts between the tags.
#86
@
10 years ago
You know, the best way to avoid that second run of the shortcode regexp would be to empty the value of $default_no_texturize_shortcodes. The one default item in there isn't even a valid shortcode. An empty list would mean there is no reason to run _wptexturize_pushpop_element() for any shortcodes. If anyone would like to second that idea, we could spin that off as another ticket.
#87
follow-ups:
↓ 88
↓ 89
@
10 years ago
Per @miqrogroove's comment my assumption in #12690 was indeed wrong. The greediness did apply to the matched group, but the alteration within the group still allowed unnecessary backtracking, and without some of the newer optimizations in older PHP versions we get a segfault. Making [^\[\]<>]
greedy in all three expressions should solve this specific backtracking issue.
However, I think we need to take a step back, revert everything to 3.9 behavior in 4.0.1 and revisit some of these ideas in 4.1:
- If we're going to match shortcodes, why aren't we using the existing cached shortcode regex?
- "Adding preg_split() inside a loop does not seem like a good direction for performance." - why not? How is it different from using preg_match() inside a loop?
- I like @azaozz's idea of splitting the regex into smaller managable, consistent and possibly reusable chunks
#88
in reply to:
↑ 87
@
10 years ago
Replying to kovshenin:
- If we're going to match shortcodes, why aren't we using the existing cached shortcode regex?
See #comment62 through #comment67. Unregistered shortcodes are allowed in order not to break Jetpack Contact.
- "Adding preg_split() inside a loop does not seem like a good direction for performance." - why not? How is it different from using preg_match() inside a loop?
Don't know, maybe due to the larger arrays created. It's more like preg_match_all
. The heuristic is to get the next match.
- I like @azaozz's idea of splitting the regex into smaller managable, consistent and possibly reusable chunks
Would have to change the spec. The []
and <>
parsing must occur at the same time, as it is now, as far as I know.
I do have a simplified algorithm if you want it:
- parse for html comments
- parse for shortcodes
- parse for html tags
It fails three tests now in trunk:
Test: '<br [gallery ...] ... />' Expect: '<br [gallery ...] ... />' Actual: '<br [gallery ...] … />' Test: '<br [[gallery ...]] ... />' Expect: '<br [[gallery ...]] ... />' Actual: '<br [[gallery ...]] … />' Test: '[ regex catches this <a href="[quote]">here</a> ]' Expect: '[ regex catches this <a href="[quote]">here</a> ]' Actual: '[ regex catches this <a href=”[quote]“>here</a> ]'
I don't consider those tests important, by the way. But []
are common in PHP links, and my patch would break:
<a href="http://example.com/?action[string]=1">
A compromise would be to restrict what can go in that string
to be considered a possible shortcode.
There is an active ticket on allowing shortcodes in more places, 24990, which my 3-way algorithm would also break I think.
Other re-arrangements of parsing comments, shortcodes and tags break more important tests.
#89
in reply to:
↑ 87
@
10 years ago
Replying to kovshenin:
However, I think we need to take a step back, revert everything to 3.9 behavior in 4.0.1 and revisit some of these ideas in 4.1:
What is your reason for a revert? The trunk version is patched and fixes all concerns raised in this ticket. If you have new concerns, speak now. :)
If we're going to match shortcodes, why aren't we using the existing cached shortcode regex?
- As kitchin pointed out, the cached shortcode regex does not match wptexturize() 3.9 behavior and breaks pre-trunk versions of Jetpack.
- The cached shortcode regex is incompatible with the HTML parsing strategy.
- The cached shortcode regex requires a recursive algorithm.
- Having a simplified pattern here is not a reason to revert all the patches for 4.0.
"Adding preg_split() inside a loop does not seem like a good direction for performance." - why not? How is it different from using preg_match() inside a loop?
You are welcome to run some benchmarks of using alternation one time vs. using preg_split against every result of a first split. It is unlikely to improve performance, and it is a red herring in terms of texturization behavior. We have no algorithmic need to do that.
I like @azaozz's idea of splitting the regex into smaller managable, consistent and possibly reusable chunks
This is happening somewhat in #29810 and does not seem relevant to 4.0.1.
#90
follow-ups:
↓ 91
↓ 92
@
10 years ago
To clarify, I'm not proposing a simplified pattern or algorithm for 4.0.1. I'm proposing to restore 3.9 behavior to stop breaking people's installs, while we take a bit more time to figure things out. The main issue in this ticket is fairly unlikely, but things like #29608, #28564 and possibly others seem pretty critical. Not sure why they were closed as duplicates of this backtracking/segfaults issue.
My concern is that r29748 and r29781 seem a bit radical for a point release. We removed matching <[^>]+>
inside the shortcodes regex, allowing <
and >
alone to match, while we explicitly prohibited that in r28727 and then allowed only "well-formed" HTML in r28773. I guess this was done to fix #29608 but I feel like there may be side-effects. Haven't seen an actual report but a [
in a shortcode attribute also worked pre-4.0 and now it doesn't, and if we remove the rest of [^\[\]]
we're pretty much back at square one :)
Re. Jetpack and other plugins that register shortcodes "just in time", obviously not in a point release, but can we look into extending the original shortcode to "support" them? Or maybe we can add a generic shortcode handler in texturize and issue a _doing_it_wrong()
? The idea here is that if we're dealing with a shortcode, it's best to handle it in a consistent way across the whole codebase.
@kitchin would love to see the code for your simplified algorithm.
#91
in reply to:
↑ 90
@
10 years ago
Replying to kovshenin:
My concern is that r29748 and r29781 seem a bit radical for a point release.
r29748 was reverted already. What is the concern here?
r29781 is the correct solution based on all feedback so far.
Haven't seen an actual report but a
[
in a shortcode attribute also worked pre-4.0 and now it doesn't
We have clear documentation that square braces are not supported in shortcodes. Solarissmoke added that warning to the Codex on 16 March 2011. Now if we want to officially support the left braces and not the right braces that's one thing, but we can't do it without breaking Jetpack or breaking the unit tests for #12690.
#92
in reply to:
↑ 90
;
follow-up:
↓ 93
@
10 years ago
Replying to kovshenin:
@kitchin would love to see the code for your simplified algorithm.
POC patch coming up. It might be good for some future work, but the changes in the unit tests show why this patch is insufficient.
One minor thing: current code treats unclosed comments differently from other unclosed html tags. Unclosed comments already have a unit test: they are treated as comments. Unclosed html tags are treated as non-html text, and I added a unit test documenting that. Not worth worrying about, it just affects whether the broken html tag is texturized or not. The browser will hide it in either case. I just added the test to show the POC can do the same thing.
#93
in reply to:
↑ 92
;
follow-up:
↓ 95
@
10 years ago
Replying to kitchin:
Unclosed html tags are treated as non-html text
My intuition says we should make a new ticket for that, mark it a defect in v1.5 and have a separate discussion. Would you like to?
#95
in reply to:
↑ 93
@
10 years ago
Replying to miqrogroove:
Replying to kitchin:
Unclosed html tags are treated as non-html text
My intuition says we should make a new ticket for that, mark it a defect in v1.5 and have a separate discussion. Would you like to?
Just to be clear, the 10.patch that landed was right for performance and matching specs and is pretty well covered by tests.
#96
follow-up:
↓ 97
@
10 years ago
Summary of recent comments:
kitchin | In favor of trunk, experimenting to find extra optimizations. |
kovshenin | Opposed to trunk because of large changes that didn't unify do_shortcode and wptexturize. |
azaozz | In favor of trunk, plus brainstorming for 4.1. |
helen | Wants better whitespace (see beautify-29557.2.diff) |
bobbingwide | Pointed out change in brackets behavior, but not concerned about it. |
nacin | Seeking feedback from mdawaffe. |
#97
in reply to:
↑ 96
;
follow-up:
↓ 99
@
10 years ago
As we are excluding strings from texturize:
- It seems we don't need to match
[[shortcode]]
, it's enough to match[shortcode]
. The extra [ ] are unaffected by texturize. - Following the same logic we don't need to match
[/shortcode]
and</tag>
. There is nothing that needs excluding from texturize in there.
#99
in reply to:
↑ 97
@
10 years ago
Replying to azaozz:
As we are excluding strings from texturize:
Escaped and closing tags both change the behavior of texturize. This is consistent with all previous versions.
#100
in reply to:
↑ 98
@
10 years ago
- Keywords commit fixed-major added
Replying to SergeyBiryukov:
In 29873:
Fix whitespace issues in [29781]. Remove a redundant comment.
Thank you Sergey and wonderboymusic for the commits. I'm putting nacin's keywords back on now.
#101
@
10 years ago
I'm going to work on offering one or two alternative patches that take slightly different strategies like just breaking old versions of Jetpack (3.2 was released today) or just breaking the plugins that use HTML inside of attributes. We are ultimately headed in the direction of harmonizing shortcodes and texturization, but I doubt that can happen in the 4.0 branch. There is also still some uncertainty about exactly which patch is the best fit at this point, and I want guys like nacin to be able to see the options exactly.
#102
in reply to:
↑ 84
@
10 years ago
Replying to miqrogroove:
Replying to kitchin:
Miqro's patch is ideal for the current tests, with one caveat: the tag regex is executed twice for each match (once in the implicit
preg_split
loop and once in theforeach
loop).
preg_match_all() might be able to structure the results better than preg_split(). I'm not sure what the speed differences would be. Also, I'm not sure there would be any elegant way to capture the parts between the tags.
Looks like PREG_OFFSET_CAPTURE gives us the data we need, and all that's needed is a tweak to the capturing groups. There might be benefits also from not splitting the string. I'm wondering if we can do a similar match_all using the full shortcode regexp and then loop through the results with some magical algebra. I'll play with that idea more as time allows.
#103
in reply to:
↑ 76
@
10 years ago
Replying to kitchin:
Miqrogroove, I did test patch 10 and all was fine. I compared it to an algorithm that first did the shortcode regex and then used strpos() to do the tag/comment parsing. The only tests that failed were the expected ones by doing it that way: shortcode-like text within html tags. Also tested it the other way (tags then shortcodes) and got expected failures. So you have the right tests as far as I see.
For the shortcodes-first strategy to work, we would have to remove the shortcodes before looking for HTML. (Assuming we continue to allow HTML in shortcodes as well). This would work only on a very limited basis, otherwise the function would become recursive and lead to DoS problems. I'll keep working on this at least to see if it runs faster or slower than the trunk code.
#104
follow-up:
↓ 105
@
10 years ago
I guess it's worth looking at my patch (more strpos, less regex) https://core.trac.wordpress.org/attachment/ticket/29557/poc_pos_29557.diff if this is the algorithm people want:
Look for 1. HTML comments, 2. then possible shortcodes, 3. then HTML tags (not comments).
It proceeds left to right and applies the three steps as it goes. Pretty much the same as preg_split() and preg_match_all(), but with a different data structure. May need to replace (2) with actual shortcodes instead of possible shortcodes.
#105
in reply to:
↑ 104
@
10 years ago
Replying to kitchin:
I guess it's worth looking at my patch (more strpos, less regex) https://core.trac.wordpress.org/attachment/ticket/29557/poc_pos_29557.diff if this is the algorithm people want:
Look for 1. HTML comments, 2. then possible shortcodes, 3. then HTML tags (not comments).
Unfortunately we have not yet reached a unanimous decision on whether to allow HTML inside of shortcodes, and if we do, we cannot process HTML comment syntax first. :( I'll look at that diff again but most likely we will work with what I've written so far. And if you have time to help, there are some parts that I could delegate.
@
10 years ago
Texturize hat trick: Full shortcode regexp, HTML parsed separately, no regexp inner loops.
#106
@
10 years ago
Was able to finish up a draft tonight. See 29557-idea-3.diff Shiny new parser in only 300 lines of code. Needs performance testing to find out if it's going to work as advertised. Needs review by core team to decide if that's the direction we want to take in 4.0.1.
This ticket was mentioned in Slack in #core by toscho. View the logs.
10 years ago
#108
@
10 years ago
That latest patch idea runs about 30x slower than trunk code :( Frustrating but not unexpected.
#109
@
10 years ago
In 29557-idea-3.3.diff I refactored the bottom half of the new parser function to eliminate all uses of the array_splice() function. The new output strategy is to build a third array from the first two arrays rather than splicing them together. It runs about 3x slower than the trunk version of wptexturize(), which is much more reasonable.
This ticket was mentioned in Slack in #core by mikengarrett. View the logs.
10 years ago
#111
@
10 years ago
If there is any more feedback about shortcodes and wptexturize, let us know now. It looks like we are going to choose one of these three options:
- Block HTML when it appears in "nested" shortcodes. My latest patch is able to do this. Yay!
- Just block all HTML in shortcodes. It may be unpopular, but it runs fast and solves the remaining problems.
- Allow HTML in all shortcodes. We do not have a full patch for this yet. At minimum, we would need a plugin-friendly way to enable recursive texturization. /wince
#112
follow-ups:
↓ 114
↓ 117
@
10 years ago
I definitely agree with the first approach, although I would rather go with the second. I'm afraid the second would alienate a lot of content managers and plugin authors.
I ran through phpunit tests put forward in 29557-idea-3.3 and can comfirm they passed. I also did my own testing with one of the main shortcode gallery plugins, Shortcodes Ultimate. The only feature that didn't work with the patch is their own version of allowing shortcodes to be attributes of other shortcodes, for example
[su_button url="{su_spoiler title='Spoiler title'}"] Button [/su_button]
The output in this case is broken, which is to be expected. The problem is that it's outputting invalid code to the front-end, so anyone that's currently using this format would have broken content.
On the other hand, shortcode nesting still worked, for example
[su_accordion] [_su_spoiler title="Spoiler title"] Spoiler content [_/su_spoiler] [_su_spoiler title="Spoiler title"] Spoiler content [_/su_spoiler] [_su_spoiler title="Spoiler title"] Spoiler content [_/su_spoiler] [/su_accordion]
All other examples I tested worked perfectly.
#113
@
10 years ago
I'm following this with interest, albeit from a bit of a distance, since I believe I may have a slightly different set of requirements to the average user.
My requirements are:
- Allow HTML in parameters in shortcodes e.g. [bw_blockquote text="A <b>bold</b> statement"]
- Allow stuff that might look like HTML in parameters in shortcodes. See #29608
- Allow shortcodes to pull in other content which may also contain shortcodes
- One day, allow shortcodes in parameters
In order to achieve the first three I've had to resequence the filter hooks for 'the_content'.
My current solution, when I need to do the above is:
remove_filter( 'the_content', 'wpautop' ); remove_filter( 'the_content', 'shortcode_unautop' ); if ( $autop ) { add_filter( 'the_content', 'bw_wpautop', 99); } else { remove_filter( 'the_content', 'bw_wpautop', 99 ); }
where bw_wpautop which can be toggled on or off as required.
My preference for texturize and wpautop would be to leave them right to the end, or not run them at all.
I'm just wondering if anyone has tried a similar approach and seen what sort of results you get from the unit test cases.
BTW. I've currently got 199 shortcodes in my development environment ( $shortcode_tags).
Slightly more than the 7 shortcodes provided by core :-)
#114
in reply to:
↑ 112
@
10 years ago
Replying to MikeNGarrett:
The only feature that didn't work with the patch is their own version of allowing shortcodes to be attributes of other shortcodes, for example
[su_button url="{su_spoiler title='Spoiler title'}"] Button [/su_button]
This is broken? Or is the plugin doing something weird internally?
This ticket was mentioned in Slack in #core by bobbingwide. View the logs.
10 years ago
#117
in reply to:
↑ 112
;
follow-up:
↓ 118
@
10 years ago
Replying to MikeNGarrett:
The only feature that didn't work with the patch is their own version of allowing shortcodes to be attributes of other shortcodes
I couldn't figure out if your example was valid. I was able to get this to work with the same plugin:
[su_button url="{su_meta key='url_for_testing'}"]Button text[/su_button]
I added a custom field named url_for_testing and set the value to http://wordpress.org/ and then the plugin created a button that linked me to the WordPress site as expected.
#118
in reply to:
↑ 117
@
10 years ago
Replying to miqrogroove:
Replying to MikeNGarrett:
The only feature that didn't work with the patch is their own version of allowing shortcodes to be attributes of other shortcodes
I couldn't figure out if your example was valid. I was able to get this to work with the same plugin:
[su_button url="{su_meta key='url_for_testing'}"]Button text[/su_button]I added a custom field named url_for_testing and set the value to http://wordpress.org/ and then the plugin created a button that linked me to the WordPress site as expected.
Looks like I had my test wrong. I've been continuing to test, but haven't been able to find any issues.
#124
follow-up:
↓ 131
@
10 years ago
- Resolution set to fixed
- Status changed from assigned to closed
I'll be opening a new ticket for 4.0.2 with remaining issues. If you have/know of remaining issues, leave a comment here.
#125
@
10 years ago
Thank you nacin for the commits. I will update Codex and things relevant to #29661. Let me know if I can help write the core post as needed.
p.s. :) http://halfelf.org/2014/resilient-responses-in-reviews/alot-of-patches/
this is the text and html of the post that doesn't work