Skip to content
This repository

Remove <p> wrappers around shortcodes #838

Closed
wants to merge 1 commit into from

2 participants

Alysson Bortoli Sam Minnée
Alysson Bortoli

Hi there,

i've added a fix to ensures that shortcodes are not wrapped in <p>...</p>.

Alysson Bortoli Remove <p> wrappers around shortcodes
Signed-off-by: Alysson Bortoli <alysson.web@gmail.com>
801b90d
Sam Minnée
Owner

I don't understand why this is necessary, can you give a big more detail about the place where this was needed? I can imagine that in many cases a <p> tag surrounding the shortcode should be left as-is.

My suspicion is that you've made a specific shortcode where it's inappropriate to leave a <p> tag wrapping it - most likely this is a shortcode that returns an entire block of its own. That being the case, we'd need to see this set up as a shortcode-specific config option that defaults to off.

Although parsing via regex is something that I'm not super-excited to see, it seems like a robust regex, and I'd particularly like to commend you on the layout and commenting of your regex - it makes it much more maintainable!

Alysson Bortoli

Hey @sminnee, thank you!

Yes that is the case. Just started developing with SS and i noticed that it wraps shortcodes in a <p> by default.

Researching i found this Shortcodes with HTML Content and there was no work around it, and i haven't found any helpful documentation, so i decided to "fix" as i think it's a bad thing to have <p> around shortcodes.

Is there any other way removing shortcodes <p> wrappers? So my HTML code can be validated, because <p> with a block container inside it is not semantic at all.

SS does not return the wrapping element so there is no way to fix it using shortcode function handler.

From: ShortcodeParser parse function line 160 returns $matches:

Array
(
    [0] => Array
        (
            [0] => Array
                (
                    [0] => [box id="enquiry-box" class="box-right"]Content goes here![/box]
                    [1] => 304
                )
            [1] => Array
                (
                    [0] => box
                    [1] => 305
                )
            [2] => Array
                (
                    [0] =>  id="enquiry-box" class="box-right"
                    [1] => 308
                )
            [3] => Array
                (
                    [0] => ]Content goes here![/box]
                    [1] => 343
                )
            [4] => Array
                (
                    [0] => Content goes here!
                    [1] => 344
                )
        )
)

I could have it as a param, but then it would not be a default feature.

[shortcode unwrap]Content[/shortcode]

But i really think it should be the other way around, so you add an attribute and ShortcodeParser add a <p>:

[shortcode autop]Content[/shortcode]

Another way of doing it is to change the default Tinymce default behavior, to not add <p> to matching [], but i think thats not the ideal solution.

Creating a shortcode most of the times developers create a .ss template file, if they don't, they could just return the shortcode content as HTML using a shortcode function handler:

public static function MyShortCodeHandler( $arguments, $content = null, $parser = null ) {
    return "<p>{$content}</p>";
}
Sam Minnée
Owner

I don't see where the <p> auto-wrapping happens. Are you sure that it isn't the WYSIWYG editor putting

tags into the HTML, rather than the shortcode parser?

Alysson Bortoli

Yes it's definitely the WYSIWYG editor. For non-WYSIWYG textareas shortcodes should be fine and not wrap it in a <p> tag.

Sam Minnée
Owner

The shortcode parsing is done on the HTML source not the plaintext as it appears in the WYSIWYG view. Frankly, this highlights the weakness of using shortcodes in a WYSIWYG editor.

If someone breaks content onto multiple lines, the HTML might end up like this:

<p>[shortcode]</p>
<p>content</p>
<p>[/shortcode]</p>

So, you say "let's strip out <p> tags". But what if <div>s were used rather than <p>s? Or one of the 1000 other variations the HTML might take?

We're doing people a disservice if we set the expectation that this is a reliable way of putting things together.

Currently, shortcodes are primarily used for the HREF attributes of <a> tags to handle URL insertion. This youtube shortcode is kind of out of date, since SS3 supports oEmbed.

Alysson Bortoli

We can have a long discussion here with a lot of arguments, but yes you're right what if

<p>[shortcode]</p>
<p>content</p>
<p>[/shortcode]</p>

But my point is, the user is not typing those <p>s so they should not be there, or be removed...

So, you say "let's strip out <p> tags". But what if <div>s were used rather than <p>s? Or one of the 1000 other variations the HTML might take?

The Tinymce editor adds <p>s by default, unless SS changes it, it should not be a problem.

My entire point is about the autops around a shortcode using WYSIWYG editor.

Sam Minnée
Owner

You seem to be missing the point of a WYSIWYG editor. It adds HTML that the user doesn't type. That's its job.

Yes, in this case, that's not intuitive, but making a system that magically detects which HTML is good and which isn't is extremely unlikely to be reliable.

I think the best fix here is to make the docs clearer that they're not intended for WYSIWYG usage except in special circumstances. The short code system is designed to work on the HTML content, not the WYSIWYG content.

I'm not going to merge this change because it sacrifices overall reliability and maintainability for a single use-case.

Sam Minnée sminnee closed this
Alysson Bortoli

All good, i should actually create a forum thread and see what other developers have to say about it. I don't see why shortcodes shouldn't be used in WYSIWYG context, specially when the default content area for a page is a WYSIWYG content.

Sam Minnée
Owner

Sweet as, http://groups.google.com/group/silverstripe-dev would be the best place to raise it.

Alysson Bortoli

Cheers :)

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

Showing 1 unique commit by 1 author.

Oct 03, 2012
Alysson Bortoli Remove <p> wrappers around shortcodes
Signed-off-by: Alysson Bortoli <alysson.web@gmail.com>
801b90d
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 50 additions and 0 deletions. Show diff stats Hide diff stats

  1. 50  parsers/ShortcodeParser.php
50  parsers/ShortcodeParser.php
@@ -158,6 +158,8 @@ public function clear() {
158 158
 	 */
159 159
 	public function parse($content) {
160 160
 		if(!$this->shortcodes) return $content;
  161
+
  162
+		$content = $this->removePTag($content);
161 163
 		
162 164
 		$shortcodes = implode('|', array_map('preg_quote', array_keys($this->shortcodes)));
163 165
 		$pattern    = "/\[($shortcodes)(.*?)(\/\]|\](?(4)|(?:(.+?)\[\/\s*\\1\s*\]))|\])/s";
@@ -208,5 +210,53 @@ protected function handleShortcode($matches) {
208 210
 			$this->shortcodes[$shortcode], 
209 211
 			$attributes, isset($matches[4][0]) ? $matches[4][0] : '', $this, $shortcode);
210 212
 	}
  213
+
  214
+	/**
  215
+	 * Don't auto-p wrap shortcodes that stand alone
  216
+	 *
  217
+	 * Ensures that shortcodes are not wrapped in <<p>>...<</p>>.
  218
+	 *
  219
+	 * @param string $content The content.
  220
+	 * @return string The filtered content.
  221
+	 */
  222
+	protected function removePTag($content) {
  223
+		if(!$this->shortcodes) return $content;
  224
+
  225
+		$tagregexp = join('|', array_map('preg_quote', array_keys($this->shortcodes)));
  226
+
  227
+		$pattern =
  228
+			  '/'
  229
+			. '<p>'                              // Opening paragraph
  230
+			. '\\s*+'                            // Optional leading whitespace
  231
+			. '('                                // 1: The shortcode
  232
+			.     '\\['                          // Opening bracket
  233
+			.     "($tagregexp)"                 // 2: Shortcode name
  234
+			.     '\\b'                          // Word boundary
  235
+			                                     // Unroll the loop: Inside the opening shortcode tag
  236
+			.     '[^\\]\\/]*'                   // Not a closing bracket or forward slash
  237
+			.     '(?:'
  238
+			.         '\\/(?!\\])'               // A forward slash not followed by a closing bracket
  239
+			.         '[^\\]\\/]*'               // Not a closing bracket or forward slash
  240
+			.     ')*?'
  241
+			.     '(?:'
  242
+			.         '\\/\\]'                   // Self closing tag and closing bracket
  243
+			.     '|'
  244
+			.         '\\]'                      // Closing bracket
  245
+			.         '(?:'                      // Unroll the loop: Optionally, anything between the opening and closing shortcode tags
  246
+			.             '[^\\[]*+'             // Not an opening bracket
  247
+			.             '(?:'
  248
+			.                 '\\[(?!\\/\\2\\])' // An opening bracket not followed by the closing shortcode tag
  249
+			.                 '[^\\[]*+'         // Not an opening bracket
  250
+			.             ')*+'
  251
+			.             '\\[\\/\\2\\]'         // Closing shortcode tag
  252
+			.         ')?'
  253
+			.     ')'
  254
+			. ')'
  255
+			. '\\s*+'                            // optional trailing whitespace
  256
+			. '<\\/p>'                           // closing paragraph
  257
+			. '/s';
  258
+
  259
+		return preg_replace( $pattern, '$1', $content );
  260
+	}
211 261
 	
212 262
 }
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.