WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 4 months ago

#10457 reopened enhancement

Parse shortcodes in text widgets by default

Reported by: ionfish Owned by: westi
Milestone: Future Release Priority: normal
Severity: normal Version: 2.8
Component: Widgets Keywords: westi-likes needs-patch
Focuses: Cc:

Description

Currently, shortcodes are only parsed within post content. It would, to my mind, be a nice enhancement to allow them to be parsed from within text widgets as well. The implementation is trivial, so the only real question is what problems this might throw up.

Attachments (5)

shortcodes.diff (509 bytes) - added by sillybean 6 years ago.
Adds shortcode processing for text widgets
10457.diff (632 bytes) - added by jamescollins 6 years ago.
Patch against trunk r12722
10457-2.diff (827 bytes) - added by sillybean 5 years ago.
patched against r13006; adds shortcode_unautop
10457.2.diff (671 bytes) - added by tmoorewp 4 years ago.
10457.3.diff (1.3 KB) - added by nacin 4 years ago.
Patch thanks to an idea from AaronCampbell.

Download all attachments as: .zip

Change History (52)

comment:1 @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.9 to Future Release

experimenting this on my end with a plugin, and it's working fine. punting pending patch.

@sillybean6 years ago

Adds shortcode processing for text widgets

comment:2 @sillybean6 years ago

  • Cc sillybean added
  • Keywords has-patch added; needs-patch removed

Patch attached.

@jamescollins6 years ago

Patch against trunk r12722

comment:3 @jamescollins6 years ago

I have attached a refreshed patch that causes shortcodes to be parsed in text widgets. It's similar to sillybean's patch, except it's for the latest (trunk) version.

It would be good to get this into 3.0 (unless anyone can think of any potential problems it may cause).

comment:4 @jamescollins6 years ago

  • Milestone changed from Future Release to 3.0
  • Summary changed from Shortcodes are not parsed in text widgets to Parse shortcodes in text widgets by default

Moving to 3.0 milestone.

Given that post writers can use shortcodes in their posts/pages, I don't think there's any problems with allowing people to use them in text widgets.

comment:5 @sillybean5 years ago

Freshened up the patch and added the shortcode_unautop filter.

@sillybean5 years ago

patched against r13006; adds shortcode_unautop

comment:6 follow-up: @technosailor5 years ago

-1

The text widget is semantically meant for TEXT. Yes, HTML could be included, but no I don't think it's proper to assume that a text widget should be parsing shortcodes too. That introduces the possibility of all kinds of nefariousness, particularly on WPMU/WPMS installs

comment:7 @sillybean5 years ago

A lot depends on what you're using shortcodes for. In most of my installations, it serves as a macro -- it gets replaced with longer text passages that need to be stored and updated centrally (like a list of rules, where you wouldn't want multiple, conflicting copies floating around).

comment:8 follow-up: @jamescollins5 years ago

We added shortcode parsing in text widgets to our WPMU installs over 6 months ago, and it hasn't caused us any problems.

The whole idea of shortcodes is to provide a way to add fancy features (eg image galleries, shopping cart buttons, etc) without needing to give blog administrators the ability to insert arbitrary HTML code.

If a shortcode is already available in a page/post, I don't think there's any reason why the administrator shouldn't also be able to include the same shortcode in a sidebar (text) widget.

For plugins, it avoids the need to create a special sidebar widget just to replicate the functionality of a widget, which means a much less cluttered widgets screen.

I have seen many existing plugins that add this shortcode parsing ability to text widgets, so it would be good to make this behavior standard in WordPress to avoid any confusion.

comment:9 in reply to: ↑ 8 ; follow-up: @jamescollins5 years ago

Replying to jamescollins:

I have seen many existing plugins that add this shortcode parsing ability to text widgets, so it would be good to make this behavior standard in WordPress to avoid any confusion.

FYI, PollDaddy is a good example of a plugin where this was recently added.

comment:10 in reply to: ↑ 9 @sillybean5 years ago

Two more use cases: BBCode and internal linking.

comment:11 in reply to: ↑ 6 @chrisscott5 years ago

Replying to technosailor:

-1

The text widget is semantically meant for TEXT. Yes, HTML could be included, but no I don't think it's proper to assume that a text widget should be parsing shortcodes too. That introduces the possibility of all kinds of nefariousness, particularly on WPMU/WPMS installs

FWIW, wp.com already allows this.

comment:12 @sillybean5 years ago

Only administrators have access to the widgets in the first place, so at some point I think we have to trust site owners not to break their own sandboxes.

comment:13 @Denis-de-Bernardy5 years ago

Marking this as tested. I've been using similar code in one of my plugins for ages. it just works.

comment:14 @Denis-de-Bernardy5 years ago

  • Keywords tested added

comment:15 @hakre5 years ago

It does not work, just search google for something like First argument is expected to be a valid callback, 'shortcode_unautop' was given and you'll see that it won't work. Looks like the callback ignite routine is not clever enough to check if something is callable before actually calling it.

Secondly it looks like that there is need to ensure a certain thing is loaded in widgets before certain filter actually can be used.

Full error message for reference:
call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'shortcode_unautop' was given in /home/tcraig/public_html/yeastinfectioncenter/wp-includes/plugin.php on line 166

Some blogs already report this as usefull for current and then 3.0. I strongly doubt that information.

So, yes it's tested, but the tests are not successfull. In 3.0 it works in 2.9.2 it does not work it looks like.

comment:16 @sillybean5 years ago

I just tested again in fresh copies of both 2.9.2 and 3.0 and it's working beautifully.

comment:17 @hakre5 years ago

Then my report must be in error. But what I'm wondering is, when a text_widget relies on that the shortcode_unautop function to be available and it is not (as the error message states) what's happening then?

comment:18 follow-up: @sillybean5 years ago

Perhaps the user is trying to make it work in 2.8.x? It looks like the function was new in 2.9.0.

comment:19 in reply to: ↑ 18 @jamescollins5 years ago

Replying to sillybean:

Perhaps the user is trying to make it work in 2.8.x? It looks like the function was new in 2.9.0.

shortcode_unautop was added in [12302] (2.9.0).

The patch seems to work fine. Not sure if it can make it into 3.0 due to future freeze though?

comment:20 @dd325 years ago

  • Milestone changed from 3.0 to Future Release

We're in feature freeze, Approaching Beta in the near future, This is an enhancement and is not a regression over the previous release. Moving to Future Release for now, with request on dev-feedback for other devs thoughts.

comment:21 @nacin5 years ago

Punting due to feature freeze. Very temping 'worksforme' candidate because it can be done via a one-line plugin.

comment:22 @nacin5 years ago

Correction, two-line plugin. Either way, the file headers take up more room.

comment:23 @jamescollins5 years ago

I agree that it should be punted to 3.1 due to feature freeze.

Even though this functionality can be added with a simple 2 line plugin, I still think it should be added to core.

Shortcodes can be used in posts/pages, so I don't see why they can't be used in text widgets.

dd32, did you mean to add the dev-feedback tag to this ticket?

comment:24 @dd325 years ago

  • Keywords dev-feedback added; shortcodes widgets removed

dd32, did you mean to add the dev-feedback tag to this ticket?

Indeed i did.. Just to track that this needs developer action.

comment:25 @jczorkmid5 years ago

  • Cc jpenney@… added

comment:26 @hew4 years ago

  • Cc hewsut@… added

+1 for adding this to core.

comment:27 @westi4 years ago

  • Keywords 3.3-early added

@tmoorewp4 years ago

comment:28 @tmoorewp4 years ago

  • Cc tim@… added

Refreshed patch against r18407.

+1 for adding to core. I already do this in a large MultiSite install and it is very useful.

comment:29 @westi4 years ago

  • Keywords westi-likes commit added; dev-feedback removed
  • Owner changed from azaozz to westi
  • Status changed from new to assigned

comment:30 @westi4 years ago

  • Keywords 3.3-early removed
  • Milestone changed from Future Release to 3.3

comment:31 @westi4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In [18592]:

Parse shortcodes in text widgets by default. Fixes #10457 props tmoorewp, sillybean, jamescollins.

comment:32 @nacin4 years ago

Follow-up: #19411, do_shortcode() runs before wpautop() in the text widget.

comment:33 @nacin4 years ago

  • Keywords revert added; has-patch tested commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunately there are too many holes that can be poked in this. For background, read #19411 and this IRC conversation.

Summarizing the problems as follows:

One, do_shortcode() would be running before our conditional wpautop() in the text widget. Shortcodes are used to running both after wpautop() and shortcode_unautop(). There are some ways around it, but it is going to be very hacky, and each hack sacrifices something else.

For example, currently anything running on the widget_text filter is always operating before wpautop(), so adding wpautop() to priority 10 will break some things, especially since plugins also on priority 10 would run after.

As a second example, I think do_shortcode() can only be run once against a block of text, otherwise it risks executing escaped shortcodes (not tested whether that is the case). So if a plugin is already adding do_shortcode() to widget_text, we could assume priority 10 and either only add with priority 10 (as trunk is now), or we'll need a delayed has_filter() check. This is why I left do_shortcode() on priority 10 and shoehorned wpautop() before it (for patches on #19411).

That's just the first problem. The second problem is what is resulting in a revert here. Core only has three shortcodes: embed, gallery, and caption. None of them are relevant for something other than a post. It looks like the caption shortcode would work (but is pretty much useless) and the gallery shortcode would just rely on whatever the post global is currently.

But, the embed code would fail. Hard. I haven't tested, but my impression would be that it would cache against the $post global. Since the specific post would constantly change for the contexts of a sidebar, the oembed response would get cached everywhere (first problem) and never be reliably cached, resulting in HTTP requests on every pageload (second problem).

Only plugin shortcodes can benefit from adding do_shortcode() to widget_text. Generally, we've used the guideline that until something benefits something that core does, it's probably not worth it. Worse, in this case, a core shortcode would result in bugs.

*

After writing all of this, upon further inspection of how the embed handlers register the [embed] shortcode, it can only work for posts, and will gracefully fail with __return_false for widgets.

However, I think there are enough problems here with wpautop/shortcodes to revert and deal with it in 3.4, given we are in RC and it will only benefit plugin shortcodes.

comment:34 @nacin4 years ago

More IRC chatter:

AaronCampbell: For 3.3 I think the answer is revert
nacin: I agree. When we find gotchas like this in RC, we missed
	something previously, it's time to back it out.
AaronCampbell: Where I'm torn is for the future.  I think this adds a
	lot of flexibility to the text widget that didn't used to be there, but
	the fact that it doesn't benefit any core shortcodes makes it a tough call
nacin: I think adding it in the future is good, it's a nice-to-have and
	really helpful.
nacin: That said, shortcodes are used to looking for a $post global.
nacin: Which bothers me a bit.
nacin: It feels plugin material.

@nacin4 years ago

Patch thanks to an idea from AaronCampbell.

comment:35 follow-up: @nacin4 years ago

10457.3.diff avoids using the filter at all. Patch neglects to remove filters from default-filters but that's easy.

This feels hacky and lame, but would keep the feature alive for 3.3. We should have a new filter *after* widget_text we can attach things to, or something.

comment:36 @nacin4 years ago

  • Keywords has-patch 2nd-opinion added

comment:37 in reply to: ↑ 35 ; follow-up: @westi4 years ago

  • Keywords 2nd-opinion removed

Replying to nacin:

10457.3.diff avoids using the filter at all. Patch neglects to remove filters from default-filters but that's easy.

This feels hacky and lame, but would keep the feature alive for 3.3. We should have a new filter *after* widget_text we can attach things to, or something.

I would rather we revert and revisit the feature than do anything hacky to keep it alive.

I think we should probably take a step back and just think about how we can make the text_widget not so lame in general - This may mean things like using a CPT to store the data and giving you full WYSIWYG and shortcode functionality that way.

This was meant to be a quick win and it turns out it wasn't it's too late in the cycle to be hacking in fixes like this.

comment:38 @westi4 years ago

In [19547]:

Revert [18592] because there are issues to solve with autop/shortcode parsing ordering before we add this. See #10457 and #19411

comment:39 in reply to: ↑ 37 @nacin4 years ago

Replying to westi:

This was meant to be a quick win and it turns out it wasn't it's too late in the cycle to be hacking in fixes like this.

100% agree.

comment:40 @nacin4 years ago

  • Keywords needs-patch added; revert has-patch removed
  • Milestone changed from 3.3 to Future Release

And punt.

comment:41 @bobbingwide4 years ago

Hi, I'd just like to say that I've done quite a bit of work on shortcodes in my plugin called oik.

I wrote a set of functions that allow a shortcode to determine whether or not to expand depending on the value of the current filter. I've termed them smart shortcodes.

There were quite a few gotchas...
I had to write code to allow for the fact that do_shortcode could get invoked multiple times - catering for this by changing the '[' to &#91 when it was not wise to expand the code.

The wpautop()code has always been annoying, inserting unwanted breaks (<br />) in all sorts of places. My feeling is that it should NOT insert a break when the line ends with a shortcode ending.

To overcome the current behaviour I often have to put shortcodes side by side in the content. This is especially bothersome when the shortcodes expand into <div>s, <span>s and/or other generated XHTML.

I also had to add special code to support shortcode expansion in wp_footer, since current_filter() returns empty in this instance.

To cut a long story short, I agree that more work needs to be done.

comment:42 @jane3 years ago

  • Milestone changed from Future Release to 3.4

Yes, please. Totally fits into the 'make your site look the way you want it to' theme of 3.4.

comment:43 @jkudish3 years ago

  • Keywords has-patch added; needs-patch removed

comment:44 @nacin3 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.4 to Future Release

Needs a new approach.

comment:45 @nofearinc20 months ago

  • Cc mario@… added

comment:46 @SergeyBiryukov18 months ago

#27108 was marked as a duplicate.

comment:47 @slackbot4 months ago

This ticket was mentioned in Slack in #themereview by emiluzelac. View the logs.

Note: See TracTickets for help on using tickets.