Allow shortcodes to be rendered without invoking Civi page

[colemanw]

This allows civi core or extensions to supply content for shortcodes without rendering a page; they can simply implement civicrm_shortcode_get_markup and return the content.

This new filter is called for any shortcode that does not specify a q parameter.

See civicrm/civicrm-core#19687

[christianwach]

@colemanw I see what you're trying to do here but I would prefer a new filter rather than repurposing an old one. Duplicating the civicrm_shortcode_get_data filter will have an impact on existing Shortcode modifications that only expect it to fire when there are multiple Shortcodes. Adding the flag is fine, but all existing callbacks will have to support it or else they may return their data every time an Afform Shortcode is rendered. Depending on their hook priority, Afform's markup may be overwritten.

My preference would be to let preprocess_atts() return FALSE and to replace the two "sanity check" code blocks here and here with something like the following:

    // The absence of a route/path needs special handling.
    if ($args === FALSE) {

      // Default markup for an improperly constructed shortcode.
      $markup = '<p>' . __('Do not know how to handle this shortcode.', 'civicrm') . '</p>';
      
      /**
       * Filter the markup for "pathless" Shortcodes.
       *
       * This filter allows plugins or CiviCRM Extensions to modify the markup used
       * to display a shortcode that has no CiviCRM route/path. This may be:
       *
       * * Accidental due to an improperly constructed shortcode or 
       * * Deliberate because a component may not require a route/path
       *
       * @since 5.36
       *
       * @param str $markup The default markup for an improperly constructed shortcode.
       * @param array $atts The shortcode attributes array.
       * @param array $args The shortcode arguments array.
       * @param str Context flag - value is either 'single' or 'multiple'.
       * @return str The modified shortcode markup.
       */
      return apply_filters('civicrm_shortcode_get_pathless_markup', $markup, $atts, $args, 'single');

    }

Obviously the second code block would have 'multiple' as the context flag. It might be better still to split this code out into a separate method so there's no duplication.

Also, please note the docblock for the filter - these are now rigorously enforced in this plugin ;-) We're planning to use phpdoc to auto-generate some developer docs at some point.

[colemanw]

Duplicating the civicrm_shortcode_get_data filter will have an impact on existing Shortcode modifications that only expect it to fire when there are multiple Shortcodes. Adding the flag is fine, but all existing callbacks will have to support it or else they may return their data every time an Afform Shortcode is rendered.

Thanks for the review @christianwach. I think you may be mistaken about the impact on existing code that uses the filter. My intention was that this change would allow all current extensions/plugins to work without modification. Could you please take a second look? I believe the new 'single' mode will only run in the case of a missing q parameter (which was previously impossible) so all current implementations of the filter will continue to work as normal:

1. They will check the $atts['component'] to see if it's the one they want to modify
2. Their targeted component must only run in "multiple" mode (because all existing components implement q).
3. They will skip processing other components such as afform (regardless of mode).

In the event that they are poorly-written and are failing to check 'component' then they are already causing problems and would be interfering with afforms in "multiple" mode regardless of whether we re-use this filter or invent a new one. My preference is still to use the existing one for both purposes. What do you think?

[christianwach]

I believe the new 'single' mode will only run in the case of a missing q parameter

Yes, true but...

They will check the $atts['component'] to see if it's the one they want to modify.

This is not necessarily true. There could be callbacks that alter any of the elements in the $data array in any way they see fit - whether per-component or globally across all components. Perhaps they're prepending or appending something to the $data['text'] element; perhaps they are initiating some other process - the point is that we don't know what the callback is doing. With a new filter, we can guarantee that there will be nothing that implements it.

Their targeted component must only run in "multiple" mode

And now callbacks such as I've described above will run if it's an Afform shortcode in "single" mode because Afform does not implement q and therefore causes civicrm_shortcode_get_data to fire.

My point is that a new filter is risk free (not sure what you mean by "current implementations of the filter will continue to work as normal" in this context) whereas re-using civicrm_shortcode_get_data is not.

[colemanw]

@christianwach ok those are good points. I've thought it through some more and have created a new filter: civicrm_shortcode_alter_conent, which allows overriding or altering the contents of any shortcode markup. It runs for every shortcode that is displayed in "single" mode, regardless of q, but it's safe to add because it doesn't exist yet so no one is using it :)

[christianwach]

@colemanw So does this mean you're adding callbacks for both civicrm_shortcode_alter_content and civicrm_shortcode_get_data in Afform? Because surely you want multiple Afforms to render when there are multiple shortcodes (because they can)? Isn't there a case for having just a single filter that "pathless" shortcodes need to implement?

[colemanw]

surely you want multiple Afforms to render when there are multiple shortcodes (because they can)?

I do? Sigh, I guess we should have all sat round and discussed this before I wrote and scrapped so much code.
I had been under the impression that the teaser-and-read-more-link style for multiple shortcodes was desired for a presentation reason but now it sounds like it was just a workaround due to technical limitations.

[christianwach]

I guess we should have all sat round and discussed this

@colemanw Never too late :-) Ping me on MM if you want to set up a chat.

[colemanw]

@christianwach I've reworked this per your suggestions.

[kcristiano]

@colemanw so for testing via r-run I am thinking:

• test existing shortcodes to be sure they are unaffected by this
• then to test this functionality - I'll need to use seachkit and afform to do this, correct?

It also looks like I'll need to enable the old editor in WP as the modal won't work any longer. Do we have documentation on what the shortcode syntax is as users cannot add via modal any longer?

[kcristiano]

@colemanw

When you submit on a WP page, there is no indication to the user that the form was successfully submitted other than a brief 'toast' alert in the upper right hand corner.

Is that expected?

[colemanw]

Sounds like you got it working.
Yes that's correct until we do more work to enable form redirects, etc.

[kcristiano]

Thanks @colemanw I have a bit more to do on existing shortcode testing, but this is working as you expect.

I am reluctant to have this merged until we have the redirect. I am not against it - but I think the form staying on the same page, no apparent visual cue that it was successful will frustrate users. I created 3 contacts before I realized what was happening - So perhaps I didn't explicitly set a dedupe rule and it did not use the unsupervised one.

I get that one has to enable Form Builder, build a form before we even get to the shortcode part - which is why I am not against this. I just am concerned about this being very alpha stage right now.

@christianwach what do you think? Am I overthinking this?

[colemanw]

Well Afforms have different purposes, and some are not actually forms with submit buttons, for example a search display listing or directory, which was the use-case I had in mind.
I think the form redirects are important but out-of-scope for this PR.

[eileenmcnaughton]

@kcristiano it does also enable searches to be embedded which is probably what people are more likely to try out given the current focus & which don't have the same issue

[kcristiano]

I did say I am not against it :) I understand we want this in early - I do see searches have a more immediate impact.

When are we cutting RC? Do we have a deadline? I like deadlines

[eileenmcnaughton]

@kcristiano rc gets cut today

[kcristiano]

@eileenmcnaughton Lacking feedback from Christian, I am OK to see this merged but would hope we could handle any concerns he raises during RC.

I think civicrm/civicrm-core#19687 should be merged first.

[eileenmcnaughton]

@kcristiano ok - I'll merge them - I doubt many people will discover them in 5,37 but it would be good to have them merged for it so they can start to experient at the bleeding edge & yeah I'm sure @colemanw will leap on any fixes

[eileenmcnaughton]

Note that we will target the rc for any follow on fixes

[christianwach]

Note that we will target the rc for any follow on fixes

That's good to hear - the functionality of Afforms displayed via the Shortcode seems... unfinished. From a usability perspective, the lack of obvious feedback and not clearing the form is going to confuse people unless addressed.

Other than that, it's great to see progress 👍

[eileenmcnaughton]

@christianwach yeah - I think afform is still marked beta which gives us a little scope to be a bit beta-ish