Better Shortcode handling #239

christianwach · 2021-02-12T17:58:36Z

Overview

Discussion and screenshots on Lab

Before

Shortcodes that have had an "action" taken on them render as if they were on the Base Page.

After

Shortcodes that have had an "action" taken on them render in the context that they were embedded in.

Technical Details

If any action is taken via a Shortcode, a query string is appended to the URL and CiviCRM-WordPress goes into "Base Page Mode". The relevant code that tries to sort all of this out can be found here. This existing code tries to distinguish between "Base Page" and "Shortcode" contexts too early in the request lifecycle, so it never really does so reliably.

What this PR does is leave the CiviCRM_For_WordPress_Basepage and CiviCRM_For_WordPress_Shortcodes classes to sort out what's going on and act accordingly once the context has been discovered beyond doubt - i.e. once the query has been set up in the wp action.

Comments

I realise that the current behaviour may now be the "expected" behaviour so this may require plenty of eyes on it to tease out the consequences for designs in the wild.

includes/civicrm.shortcodes.php

homotechsual · 2021-02-12T18:13:11Z

This looks okay to me and testing on a BuildKit WPMaster build doesn't reveal any issues with vanilla Civi and out-of-the-box WP themes - which is a good start :-)

christianwach · 2021-02-12T18:23:20Z

@MikeyMJCO Thanks for testing!

It's likely to be themes with page layout editors (e.g. Visual Composer) that run into the problem. The default themes rarely sub-divide the page content for layout purposes.

christianwach · 2021-02-12T18:24:33Z

I think every built-in Shortcode is going to have to be tested since they have a variety of behaviours. Personal Campaign Pages, for example, are more complex than Profile forms.

agh1 · 2021-02-13T00:23:08Z

I just tried it out and this affects two situations:

You fail validation or otherwise do something that reloads the form. This is the example in your comment on the ticket.
You proceed through to the confirmation page and/or thank-you page.

I see absolutely no problem with the first situation.

For the second, however, I don't necessarily mind this in the abstract but it will almost certainly cause (minor, resolvable) problems for certain sites. If a non-hijacked page has an event form on it and intro and/or footer text in the page, that text is probably only relevant for the first step in the registration process. When that intro text appears at the top of the confirmation page and thank-you page, it will probably confuse the person filling out the form.

The same thing happens when you click the register now link from an event info page.

The workaround is generally simple: if you have information that's specific to the initial form, put it in the initial form rather than the page. However, there certainly are plenty of sites that don't do that, and they would need to fix it when they upgrade. Also, this would raise the question of what's the point of not hijacking the page if you need to clear everything out from the page.

christianwach · 2021-02-13T13:57:59Z

@agh1 Thanks for reviewing, Andrew.

If a non-hijacked page has an event form on it and intro and/or footer text in the page, that text is probably only relevant for the first step in the registration process.

Yes, I agree, many sites could be built that way. But shouldn't that text more properly be defined in the "Online Registration" tab on the CiviEvent?

The same thing happens when you click the register now link from an event info page.

The "text" I show in the screenshots on Lab is above and below the Shortcode in the WordPress Page content. That could, however, be anything - layout, e.g. other columns of content or similar; design e.g. graphics; a menu or whatever. Consider the following Page content - I've created the layout in the content the way that Visual Composer, Divi or Elementor would.

Here's the design of an Event Info page:

With this PR, the registration page retains the design:

As does the Thank You page:

Without this PR the layout changes to:

Although this is "expected" (since that's the way it has always worked) it's also unexpected to anyone fresh to CiviCRM.

The workaround is generally simple: if you have information that's specific to the initial form, put it in the initial form rather than the page. However, there certainly are plenty of sites that don't do that, and they would need to fix it when they upgrade.

Yes, I agree - I think this is more a question of documentation and managing expectations.

Also, this would raise the question of what's the point of not hijacking the page if you need to clear everything out from the page.

Again, I agree. I think this actually makes the distinction between hijack="0" and hijack="1" clearer and more consistent. At present hijack="0" actually means "do not hijack the page unless an action has been taken, in which case hijack the page".

agh1 · 2021-02-13T17:31:30Z

Yeah I guess it largely depends upon what we think of as content. Actual content is likely to be specific to the initial form only. However, for better or worse, lots of WP sites have page content that would be more like theming, strictly speaking, and should persist through the process. This PR accommodates the latter at the expense of the former. It's probably the right call if we were starting from scratch, but we've got sites that are used to the former working and the latter failing. Would it be possible to have a URL param that forces hijack? Even if nothing in this PR uses it, maybe an extension or form option could be set so that it adds the param as you proceed, making things work the old way if you want. Organizations are rightfully sensitive about every change in donation forms, and as-is, this would force many to rework their site upon upgrade. Folks will miss the memo or not have the capacity to do it, and they'll be angry. Feb 13, 2021 8:58:16 AM Christian Wach <notifications@github.com>:

…

@agh1[https://github.com/agh1] Thanks for reviewing, Andrew. > If a non-hijacked page has an event form on it and intro and/or footer text in the page, that text is probably only relevant for the first step in the registration process. > Yes, I agree, many sites could be built that way. But shouldn't that text more properly be defined in the "Online Registration" tab on the CiviEvent? > The same thing happens when you click the register now link from an event info page. > The "text" I show in the screenshots on Lab is above and below the Shortcode in the WordPress Page content. That could, however, be anything - layout, e.g. other columns of content or similar; design e.g. graphics; a menu or whatever. Consider the following Page content - I've created the layout in the content the way that Visual Composer, Divi or Elementor would. Here's the design of an info page: [https://user-images.githubusercontent.com/726936/107850793-0e5c1000-6dfd-11eb-888e-74d383cde2ca.png###948x0:false###][Screenshot 2021-02-13 at 13 11 13][https://user-images.githubusercontent.com/726936/107850793-0e5c1000-6dfd-11eb-888e-74d383cde2ca.png] With this PR, the registration page retains the design: [https://user-images.githubusercontent.com/726936/107850794-1025d380-6dfd-11eb-9aac-26b7fc39c430.png###943x0:false###][Screenshot 2021-02-13 at 13 11 32][https://user-images.githubusercontent.com/726936/107850794-1025d380-6dfd-11eb-9aac-26b7fc39c430.png] As does the Thank You page: [https://user-images.githubusercontent.com/726936/107851428-b378e780-6e01-11eb-9d5e-2fc8655332db.png###948x0:false###][Screenshot 2021-02-13 at 13 44 28][https://user-images.githubusercontent.com/726936/107851428-b378e780-6e01-11eb-9d5e-2fc8655332db.png] Without this PR the layout changes to: [https://user-images.githubusercontent.com/726936/107850821-567b3280-6dfd-11eb-93a9-44435b47ebb3.png###938x0:false###][Screenshot 2021-02-13 at 13 13 45][https://user-images.githubusercontent.com/726936/107850821-567b3280-6dfd-11eb-93a9-44435b47ebb3.png] > The workaround is generally simple: if you have information that's specific to the initial form, put it in the initial form rather than the page. However, there certainly are plenty of sites that don't do that, and they would need to fix it when they upgrade. > Yes, I agree - I think this is more a question of documentation and managing expectations. > Also, this would raise the question of what's the point of not hijacking the page if you need to clear everything out from the page. > Again, I agree. I think this actually makes the distinction between hijack="0" and hijack="1" clearer and more consistent. At present hijack="0" actually means "do not hijack the page unless an action has been taken, in which case hijack the page". — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub[#239 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAM2XR26UR7YZXAJAGBKDTLS62APPANCNFSM4XRC4MAA]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAD8AAAA/CAYAAABXXxDfAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAABcSURBVGgF7dABDQAAAMKg909tDwcRKAwYMGDAgAEDBgwYMGDAgAEDBgwYMGDAgAEDBgwYMGDAgAEDBgwYMGDAgAEDBgwYMGDAgAEDBgwYMGDAgAEDBgwYMGDgZGA+QwABeHisFQAAAABJRU5ErkJggg==###24x24:true###][Tracking image][https://github.com/notifications/beacon/AAM2XR5RFJKEBVELWX5OTFLS62APPA5CNFSM4XRC4MAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFZUNTHQ.gif]

christianwach · 2021-02-13T21:20:45Z

Organizations are rightfully sensitive about every change in donation forms, and as-is, this would force many to rework their site upon upgrade.

I guess there's another option - which would be to add another setting, e.g. hijack="2" perhaps labelled "True Non-Hijack" or something. That would leave existing installs with their current behaviour whilst adding the functionality of this PR as an option for those who need it. Might be hard to explain, however.

Possible wording of the new options:

christianwach · 2021-02-13T21:27:34Z

Actual content is likely to be specific to the initial form only.

I wonder for how many installs this is true? It would be interesting to find out, though I can imagine it being hard to do so.

My guess would be that the majority of Shortcodes are used with hijack-"1" where the main purpose of the Shortcode is to provide a more suitable URL for the CiviCRM content than the one provided by the Base Page, e.g. /donate or /join. That's the only way I've ever used Shortcodes - but I'm aware that's anecdote not data.

kcristiano · 2021-02-16T13:37:26Z

https://lab.civicrm.org/dev/wordpress/-/issues/90#note_54161

I've done and r-run and this is better merged than not. As there is lively discussion I'll hold off merging for now.

agh1 · 2021-02-16T17:06:28Z

@christianwach to be clear I wasn't really suggesting an option to revert to precisely the status quo ("first page load"). I think few people like/expect that validation errors and coupon codes should cause hijack to commence. Instead, it's really just the subsequent steps in the registration process, and CiviCRM uses URL params to identify which step you're on, so that seems like a good mechanism to support in CiviCRM_For_WordPress_Basepage::register_hooks(). Basically, when you're deciding it's a basepage, immediately assume Yes if civibasepage=1 or equivalent is in the request. If this PR can support that, the rest could be handled by extension or independent core changes that would support a choice.

agh1 · 2021-02-16T17:28:45Z

includes/civicrm.basepage.php

@@ -348,6 +332,17 @@ public function basepage_handler($wp) {

        global $post;

+        // Skip if this not the Base Page.
+        if ($basepage->ID !== $post->ID) {


This here seems to be the key change in behavior: instead of defaulting to basepage, this only goes to basepage mode if you're on the basepage as specified in the CiviCRM settings. I imagine this is where to add something where the if is FALSE if there's a URL param specifying to use basepage mode.

Incidentally, I have no idea whether it's common to use another page as your basepage, but I wanted to flag the following change in behavior. The examples below all assume that the default page civicrm exists and is the basepage set in CiviCRM. Also, there is another page other that exists.

Before (master)

URL Rendered

http://example.org/civicrm/?civiwp=CiviCRM&q=civicrm/contribute/transact&reset=1&id=1 CiviCRM Contribution page ID 1, template of civicrm, no page content

http://example.org/other/?civiwp=CiviCRM&q=civicrm/contribute/transact&reset=1&id=1 CiviCRM Contribution page ID 1, template of other, no page content

After (with this PR)

URL Rendered

http://example.org/civicrm/?civiwp=CiviCRM&q=civicrm/contribute/transact&reset=1&id=1 CiviCRM Contribution page ID 1, template of civicrm, no page content

http://example.org/other/?civiwp=CiviCRM&q=civicrm/contribute/transact&reset=1&id=1 The normal page content and template on other, with nothing from CiviCRM

The only use case I've encountered of this is a site that had a special template for the conference section of their website and they wanted the registration form to have that look. Of course, they could use a shortcode for this, instead. Unlike the other concern about content on a shortcode page, I think this could be easily resolved with an upgrade message.

However, it might be good to run through some of the scenarios @christianwach and @kcristiano were thinking of in civicrm/civicrm-core#17698 where the recorded basepage may not exist neatly in WP multisite.

@agh1 Thanks for the analysis.

Yes, you're right; this PR does prevent rendering of CiviCRM content in the /other URL scenario (i.e. not on the Base Page) unless embedded via a Shortcode, of course. Personally, I think this kind of URL should not function, though I appreciate that people may be using it because it does.

For the Base Page URL... I'd imagine that most Base Page requests are made using Clean URLs these days as it's been the default for quite some time. But as you point out, nothing changes for these Base Page URLs either way.

I can't at the moment think of any impact when a Base Page doesn't exist - usually that means either (a) CiviCRM is incorrectly installed and it should have a Base Page or (b) it has no need for front-end content or (c) it's multisite and there's an appropriately configured install that doesn't need a Base Page. Happy to be proved wrong :-)

@agh1 Does 659b8e4 solve this for you?

@christianwach yep, see comment on the main thread!

christianwach · 2021-02-17T14:43:44Z

civibasepage=1

@agh1 What's your thinking here? Are you suggesting an additional param?

christianwach · 2021-02-17T14:47:53Z

@agh1 Okay, so on further reflection, what I think you're saying is that you'd like certain scenarios (e.g. failed form validation) to remain in "Shortcode mode" whilst other scenarios should trigger "Base Page mode". Is that correct?

agh1 · 2021-02-17T21:55:26Z

@agh1 What's your thinking here? Are you suggesting an additional param?

@christianwach yes, it would be an addtional param that, if truthy, would make it be in basepage mode. Basically, instead of

        // Skip if this not the Base Page.
        if ($basepage->ID !== $post->ID) {

do this

        // Skip if this not the Base Page.
        if ($basepage->ID !== $post->ID && empty($_REQUEST['civibasepage'])) {

Basically I'm saying that the general behavior of this PR is great for most sites and it's not your responsibility to take care of all the folks with intro content above their shortcodes, but those people are out there and having something like this would make it possible to extend CiviCRM to simulate the status quo.

Nobody expects that failed validation should trigger base page mode, even though that happens to be the status quo. A bunch of people expect that proceeding through registration/donation/etc. should trigger base page mode, since that's the status quo and semi-reasonable (even though what you're proposing is more reasonable). I don't want to saddle you with dealing with those scenarios, but I don't want to leave folks high and dry.

A param is what seems like the most straightforward way to accomplish this, since the places we'd want to do it are proceeding through form steps that change params, but any other mechanism would also be fine.

agileware-justin · 2021-02-18T02:34:02Z

Nobody expects that failed validation should trigger base page mode, even though that happens to be the status quo.

Absolutely agree with this point.

A bunch of people expect that proceeding through registration/donation/etc. should trigger base page mode, since that's the status quo and semi-reasonable (even though what you're proposing is more reasonable).

It would be a great improvement for CiviCRM shortcode to behave just like any other WordPress shortcode, just stay on the same page that it was embedded on. This enables you to have consistent Page Templates selected, rather than flipping between the CiviCRM base page and page where the shortcode was embedded. Keep the sidebars in place, menus etc. keeping the context of the page as is.

I am OK with changing the status quo here, because it's currently not great.

includes/civicrm.basepage.php

christianwach · 2021-02-18T13:12:14Z

an additional param that, if truthy, would make it be in basepage mode

@agh1 Okay, so I've taken a more WordPress-y approach to this in my latest commit. I'm not keen on forcing the usage of a particular query var on developers - standard practice in these situations is to add a filter. You can now use any query var that you choose to implement in $_REQUEST in your callback to the civicrm_force_basepage_mode filter, e.g.

add_filter( 'civicrm_force_basepage_mode', 'my_civicrm_force_basepage_mode', 10, 2 );
function my_civicrm_force_basepage_mode( $allow, $post ) {

	// Allow "Base Page mode" on a particular page or post.
	if ( $post->ID === $my_special_page->ID ) {
		return TRUE;
	}

	// Allow "Base Page mode" in the presence of a query var.
	if ( ! empty( $_REQUEST['civibasepage'] ) ) {
		return TRUE;
	}

	// Otherwise pass through.
	return $allow;

}

I think this provides greater flexibility.

agh1 · 2021-02-24T18:37:16Z

The latest change should work well. I was hesitant at first because I was thinking it would be challenging to implement a per-event, per-contribution form, etc. override for this, but upon reflection it works better because the override could potentially be per-post, which is more specific to the issue. (For example, maybe the donation form shortcode appears in two spots, and one should work like the new normal and retain page content while the other should work like the old normal and go into basepage mode as you proceed.) And of course, a plugin could go check for the contribution page ID from the post and decide according to that.

At this point, this PR looks good and we just need a corresponding one for civicrm-core that adds an upgrade message explaining all of this.

kcristiano · 2021-02-24T20:28:38Z

At this point, this PR looks good and we just need a corresponding one for civicrm-core that adds an upgrade message explaining all of this.

I am not sure what needs to be detailed here. This looks more like we are fixing unintended behavior. I see upgrade notices as reporting changes that could break sites and that a user should notice and maybe delay the upgrade or make changes before upgrading. I am not sure what action item we'd give the user.

@agh1 Can you elaborate as to what I've missed?

BTW, I do think we need to update the docs for certain. Just unsure of an upgrade notice.

agh1 · 2021-02-24T20:53:08Z

@kcristiano it's totally unintended behavior, but there are definitely people out there who have been putting their event/contribution intro content in the page. I was thinking of something like this:

The behavior of front-end CiviCRM forms inserted via shortcode in WordPress pages and posts has changed. If the shortcode does not override page content, any page content surrounding the shortcode will now remain in place on every step of the form submission process--through to confirmation and thank you. Content that is specific to the first step of the form should instead go in the introduction of the CiviCRM form. Read more in the documentation for options if this is a problem.

The action item for most people is simply to move their intro text out of WP and into Civi. The docs can explain that filter.

kcristiano · 2021-02-24T21:02:08Z

OK. I see your point. No user will read the release notes, so we want a place to alert them on upgrade.

@christianwach what do you think?

agileware-justin · 2021-02-24T22:07:24Z

3 thumbs up from me 👍 👍 👍

christianwach · 2021-02-25T10:32:42Z

I'm happy for there to be an upgrade notice.

christianwach · 2021-10-15T09:18:25Z

Closing in favour of #258

Better Shortcode handling

dd6be1c

christianwach force-pushed the shortcode branch from af94c0a to dd6be1c Compare February 12, 2021 18:07

homotechsual reviewed Feb 12, 2021

View reviewed changes

includes/civicrm.shortcodes.php Outdated Show resolved Hide resolved

Update malformed Shortcode warning

d395ada

agh1 reviewed Feb 16, 2021

View reviewed changes

Allow "Base Page mode" to be forced

659b8e4

kcristiano reviewed Feb 18, 2021

View reviewed changes

includes/civicrm.basepage.php Outdated Show resolved Hide resolved

Correct CiviCRM version when filter was introduced

0b557e0

kcristiano added the needs-documentation label Feb 24, 2021

christianwach mentioned this pull request Oct 15, 2021

Introduce "Shortcode Display Mode" setting and functionality #258

Merged

christianwach closed this Oct 15, 2021

christianwach deleted the shortcode branch October 15, 2021 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better Shortcode handling #239

Better Shortcode handling #239

christianwach commented Feb 12, 2021 •

edited

Loading

homotechsual commented Feb 12, 2021

christianwach commented Feb 12, 2021

christianwach commented Feb 12, 2021

agh1 commented Feb 13, 2021

christianwach commented Feb 13, 2021 •

edited

Loading

agh1 commented Feb 13, 2021 via email

christianwach commented Feb 13, 2021 •

edited

Loading

christianwach commented Feb 13, 2021 •

edited

Loading

kcristiano commented Feb 16, 2021

agh1 commented Feb 16, 2021

agh1 Feb 16, 2021 •

edited

Loading

christianwach Feb 17, 2021 •

edited

Loading

christianwach Feb 24, 2021

agh1 Feb 24, 2021

christianwach commented Feb 17, 2021

christianwach commented Feb 17, 2021

agh1 commented Feb 17, 2021

agileware-justin commented Feb 18, 2021

christianwach commented Feb 18, 2021

agh1 commented Feb 24, 2021

kcristiano commented Feb 24, 2021

agh1 commented Feb 24, 2021

kcristiano commented Feb 24, 2021

agileware-justin commented Feb 24, 2021

christianwach commented Feb 25, 2021

christianwach commented Oct 15, 2021

URL	Rendered
http://example.org/civicrm/?civiwp=CiviCRM&q=civicrm/contribute/transact&reset=1&id=1	CiviCRM Contribution page ID 1, template of `civicrm`, no page content
http://example.org/other/?civiwp=CiviCRM&q=civicrm/contribute/transact&reset=1&id=1	CiviCRM Contribution page ID 1, template of `other`, no page content

Better Shortcode handling #239

Better Shortcode handling #239

Conversation

christianwach commented Feb 12, 2021 • edited Loading

Overview

Before

After

Technical Details

Comments

homotechsual commented Feb 12, 2021

christianwach commented Feb 12, 2021

christianwach commented Feb 12, 2021

agh1 commented Feb 13, 2021

christianwach commented Feb 13, 2021 • edited Loading

agh1 commented Feb 13, 2021 via email

christianwach commented Feb 13, 2021 • edited Loading

christianwach commented Feb 13, 2021 • edited Loading

kcristiano commented Feb 16, 2021

agh1 commented Feb 16, 2021

agh1 Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Before (master)

After (with this PR)

christianwach Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

christianwach Feb 24, 2021

Choose a reason for hiding this comment

agh1 Feb 24, 2021

Choose a reason for hiding this comment

christianwach commented Feb 17, 2021

christianwach commented Feb 17, 2021

agh1 commented Feb 17, 2021

agileware-justin commented Feb 18, 2021

christianwach commented Feb 18, 2021

agh1 commented Feb 24, 2021

kcristiano commented Feb 24, 2021

agh1 commented Feb 24, 2021

kcristiano commented Feb 24, 2021

agileware-justin commented Feb 24, 2021

christianwach commented Feb 25, 2021

christianwach commented Oct 15, 2021

christianwach commented Feb 12, 2021 •

edited

Loading

christianwach commented Feb 13, 2021 •

edited

Loading

christianwach commented Feb 13, 2021 •

edited

Loading

christianwach commented Feb 13, 2021 •

edited

Loading

agh1 Feb 16, 2021 •

edited

Loading

christianwach Feb 17, 2021 •

edited

Loading