(dev/core#4433) - Implement Civi::url() with prefixes and OOP enhancements #26861

totten · 2023-07-17T10:51:44Z

Overview

This is a concrete demonstration of some ideas from https://lab.civicrm.org/dev/core/-/issues/4433.

Before

This function exists:

CRM_Utils_System::url(NULL, NULL, FALSE, 1, '', TRUE, __CLASS__, NULL, TRUE);

And that does... something. Have fun figuring it out...

After

echo Civi::url('frontend://civicrm/event/info?id=123&reset=1');

echo Civi::url('//civicrm/profile/view?reset=1')
  ->addQuery(['gid' => $x, 'id' => $y])

echo Civi::url('assetBuilder://crm-l10n.js')
  ->addQuery(['locale' => $x])

(Docblock has more examples...)

Technical Details

In this rendition:

The entry-point (frontend/backend/etc) is a URI scheme (frontend://).
It respects URI convention for inheriting current scheme (//foo.txt means "get foo.txt with current scheme)
The list of URI schemes is borrowed from other places in core -- e.g. the Guzzle-test system has frontend://ROUTE and backend://ROUTE. Some of the asset/resource management supports assetBuilder://NAME and ext://EXT/FILE.

civibot · 2023-07-17T10:51:46Z

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers

Civi/Core/Url.php

colemanw · 2023-07-17T12:04:50Z

Civi/Core/Url.php

+
+        // (p)lain text encoding
+        case 'p':
+          $this->htmlEscape = FALSE;


Ooh, an undocumented flag that overrules htmlize! Yet another blow to its fragile ego... perhaps leading it to the brink of a well-deserved existential crisis.

@totten this flag is still undocumented. Should it be removed?

The "undocumented" note had confused me, but I see now: it's documented on Civi::url(...$flags...) but not Url::useFlags($flags). That's easy to cleanup.

I should take a stab at the Smarty side before deciding whether to remove that flag. My suspicion is that it'll make it easier to tune Smarty. (Quick estimate: in core, there are ~530 places where Smarty wants HTML and ~130 places where it wants plain text.)

Civi/Core/Url.php

…tent rule for escaping inputs.

totten · 2023-07-25T08:57:10Z

@colemanw So I've done a bunch of updates

Add test-coverage and many more docblocks.
Add support for Angular-style URL data (backend://civicrm/a/#/mailing/[id]?angularDebug=1)
Add better support for static asset URLs (eg asset://[civicrm.packages]/foobar.js)
Add support for metadata-based routing (default://civicrm/admin)
Define a consistent policy on escaping:
- Whenever you pass in a string (setQuery('id=123&msg=hello+world')), then treat it as a literal URL fragment.
- Whenever you pass an array (setQuery(['id' => 123, 'msg' => 'hello world'])), then we apply escaping.

Define a Smarty utility {url}...{/url}:

<a href="{url}backend://civicrm/admin?reset=1{/url}">Administration</a>

{url assign=adminUrl}backend://civicrm/admin?reset=1{/url}
<a href="{$adminUrl}">Administration</a>

<a href="{url contact=100 profile=200}current://civicrm/profile/view?id=[contact]&gid=[profile]{/url}">

<a href="{url 1=$contacts[$i].contact_id 2=$profile.id}current://civicrm/profile/view?id=[1]&gid=[2]{/url}">

So overall, from my POV, it has all the functionality I've expected.

There are a couple possible areas of further consideration:

The Second Hard Problem of Computer Science - The scheme names (frontend://, backend://, asset://, assetBuilder://) have a sort of logic to them. But are they good names?... frontend:// and backend:// are kinda long. Maybe asset:// should be like resource:// or res://. Yaddayadda.
Trial Conversions - Do a batch of conversions (CRM_Utils_System::url() to Civi::url(); and {crmURL} to {url}). See if they feel alright.
URL Contexts/Default Options - So there's this recurring bug where you make a URL, but sometimes it doesn't look the way you expect, and so you have to go back and finesse options.
- Basically, when running code, you're in some operative context. And I think we come with different intuitions about how URLs should be formulated in each context.
  - Displaying a web-page. Here, relative URLs are nice. In case of ambiguity, you should lean toward keeping links in the same UI (i.e. frontend-pages tend to link to frontend-pages).
  - Generating an email message. Here, absolute URLs are nice. In case of ambiguity, you should lean toward frontend UI (even though the email might be generated from backend UI, from automated job, etc)
  - Running admin/dev tools (cv url, cv http). Here, absolute URLs are nice. You're not in frontend:// or backend://, so you could lean on metadata instead (default://).
- I added some stuff to support these intuitions. This seems to work for cv. But the email-generating scenario seems important, and I haven't tried it. The concept feels tentative until the email side has been done.

Those are all kind of debatable. Anyway, I'm not inclined to self-block any more... so I've removed "Draft/WIP" flag.

christianwach

(Sorry, still not clear how the review process works here)

str_starts_with() is PHP 8.0+ so we'll need a shim to continue support for PHP 7.x.

Civi/Core/Url.php

christianwach · 2023-07-25T12:02:45Z

Civi/Core/Url.php

+      case 'service':
+        $result = $userSystem->url($this->getPath(), $this->getQuery(), $preferFormat === 'absolute', $this->composeFragment(), TRUE, FALSE, FALSE);
+        break;
+
+      case 'backend':
+        $result = $userSystem->url($this->getPath(), $this->getQuery(), $preferFormat === 'absolute', $this->composeFragment(), FALSE, TRUE, FALSE);
+        break;


Am i right in thinking that routing to $userSystem->url() here provides an opportunity (sometime in the future) to replace it with something less problematic?

@christianwach I do like the idea of $userSystem having some more influence/visibility on the $scheme. For example, I don't think it's great for frontend:// and service:// to be conjoined like that. I'll add on some ideas for that...

@christianwach So does 59af62f seem better/worse/same? It delegates to $userSystem->getRouteUrl() instead of $userSystem->url(). The main differences are:

UF::getRouteUrl() receives the name (frontend, backend, service) instead of multiple bools. It can potentially receive other/custom schemes if the UF has other environments.

UF::getRouteUrl() doesn't have to deal with the fragment or absolute/relative flag. (These seem amenable to generic handling.)

Another thing the UF might influence is the interpretation of current:// (ie detectScheme()). With the current PR, the UF-integration could set civicrm_url_defaults before invoking the page.

So does 59af62f seem better/worse/same?

@totten Better in that the inscrutable boolean params are somewhat hidden. Nice one :)

christianwach · 2023-07-25T13:01:28Z

@totten Thanks for starting to address this issue. I like the direction of travel - it's certainly a more readable way of collecting the various pieces of the URL together. Looks promising.

Coming from a WordPress-integration perspective, my focus is less on how the pieces are collected and more on how those URLs are subsequently built for use in the various CMS contexts, i.e.

WordPress admin
The Base Page with or without Clean URLs
Shortcodes - where URLs are based on the Permalink structure plus query vars
Blocks - which in this plugin call the QuickForm classes directly but may use asset:// or assetBuilder:// in future
WordPress multilingual plugins (which use the URL scheme in various ways to encode the desired language into it)

For now, it looks as though I can still work on the logic in CRM_Utils_System_WordPress::url() to make it adaptable to the various scenarios that it has to handle... just leaving this here as a reminder in case work is started on URL rendering that does not route via the current $userSystem->url() method(s).

…Fix some edge-case bugs.

totten · 2023-07-26T01:31:15Z

Thanks for that last comment @christianwach. Those are some interesting contexts. Thinking about #2 vs #3 -- both are front-end-ish, but #2 is a singular frontend -- and #3 has n-many variations. (EDIT: This was written as a statement, but in my head it was intoned as a question. :) Just trying to make sure I follow the differences.) Which means:

While rendering web UI for #2 or #3, you could use url('current://...') and expect links with matching structure.
While processing a background job (say, a sending a stock email with a link to update a recurring contribution), it's much easier to link to #2 than #3. For #2, you can simply interpret url('frontend://civicrm/contribute/updaterecur') as https://example.com/civicrm/contribute/updaterecur. But if you want the emails to point to one of n-many WP pages that support shortcodes, then you need to edit the email-template (or define some complicated override).

colemanw · 2023-09-06T23:54:34Z

I think we've sat on this one long enough and it has good test coverage. Let's start using it!

civibot bot added the master label Jul 17, 2023

totten changed the title ~~(WIP/Draft) - Implement Civi::url() with prefixes and OOP enhancements~~ (WIP/Draft) (dev/core#4433) - Implement Civi::url() with prefixes and OOP enhancements Jul 17, 2023

colemanw reviewed Jul 17, 2023

View reviewed changes

Civi/Core/Url.php Show resolved Hide resolved

colemanw reviewed Jul 17, 2023

View reviewed changes

Civi/Core/Url.php Show resolved Hide resolved

totten force-pushed the master-url branch from 3e6e0cc to 4ae8f02 Compare July 21, 2023 05:34

totten added 7 commits July 24, 2023 01:12

Implement Civi::url()

b42c6d9

Civi::url() - Allow environments to change default scheme and format

f8eb68a

Civi::url() - Add support for vars

d65ecc5

(NFC) Civi::url() - Various docblocks

5246994

Civi::url() - Rename (p) to (t)

840c055

Civi::url() - Add option for resCacheCode

6437be7

Civi::url() - Add support for path-variables

a6c51f6

totten force-pushed the master-url branch from 4ae8f02 to 457325b Compare July 24, 2023 08:13

totten mentioned this pull request Jul 24, 2023

(dev/core#4433) WordPress::url() - By default, choose frontend or backend based on route #25476

Closed

Civi::url() - Add test-coverage

cc6111f

totten force-pushed the master-url branch from 457325b to 6391570 Compare July 25, 2023 00:04

Civi::url() - Split $fragment and $fragmentQuery. Define clear/consis…

da37ead

…tent rule for escaping inputs.

totten force-pushed the master-url branch from 6391570 to f244b78 Compare July 25, 2023 02:04

Civi\Core\Url - Also support JSON serialization

794c107

totten force-pushed the master-url branch from f244b78 to 794c107 Compare July 25, 2023 03:19

totten added 3 commits July 24, 2023 22:29

UrlTest - Add coverage for variable-substitution

4a9c147

Civi\Core\Url - Allow dynamic variables

da80590

Smarty - Add support for {url}frontend://civicrm/profile{/url} (etal)

76b2a80

totten force-pushed the master-url branch from a4c6c8e to 76b2a80 Compare July 25, 2023 05:59

totten added 3 commits July 25, 2023 00:52

Mark civicrm/asset/builder as public

475b114

CRM_Core_Menu - Add helper method, isPublicRoute($path).

418cdfc

Civi::url() - Add support for default:// URIs

9477885

totten changed the title ~~(WIP/Draft) (dev/core#4433) - Implement Civi::url() with prefixes and OOP enhancements~~ (dev/core#4433) - Implement Civi::url() with prefixes and OOP enhancements Jul 25, 2023

totten added the has-test label Jul 25, 2023

totten mentioned this pull request Jul 25, 2023

BootTrait - Define a preference for absolute URLs in default:// civicrm/cv#171

Open

totten marked this pull request as ready for review July 25, 2023 08:57

christianwach suggested changes Jul 25, 2023

View reviewed changes

Civi/Core/Url.php Show resolved Hide resolved

christianwach reviewed Jul 25, 2023

View reviewed changes

totten added 3 commits July 25, 2023 16:02

Extract CRM_Utils_System_Base::getRouteUrl()

59af62f

Civi::url() - Simplify/normalize "fragment" and "relative" handling. …

4987909

…Fix some edge-case bugs.

Civi::url() - Allow building new URL from empty. Update comments.

d5a81c8

totten force-pushed the master-url branch from 35e1688 to d5a81c8 Compare July 26, 2023 00:48

colemanw merged commit 18b4ca2 into civicrm:master Sep 6, 2023

totten deleted the master-url branch September 8, 2023 01:53

colemanw mentioned this pull request Sep 24, 2023

Delete unused Smarty block serialize #27619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(dev/core#4433) - Implement Civi::url() with prefixes and OOP enhancements #26861

(dev/core#4433) - Implement Civi::url() with prefixes and OOP enhancements #26861

totten commented Jul 17, 2023 •

edited

Loading

civibot bot commented Jul 17, 2023

colemanw Jul 17, 2023

colemanw Jul 21, 2023

totten Jul 21, 2023

totten commented Jul 25, 2023 •

edited

Loading

christianwach left a comment

christianwach Jul 25, 2023

totten Jul 25, 2023

totten Jul 26, 2023

christianwach Aug 23, 2023

christianwach commented Jul 25, 2023

totten commented Jul 26, 2023 •

edited

Loading

colemanw commented Sep 6, 2023

(dev/core#4433) - Implement Civi::url() with prefixes and OOP enhancements #26861

(dev/core#4433) - Implement Civi::url() with prefixes and OOP enhancements #26861

Conversation

totten commented Jul 17, 2023 • edited Loading

Overview

Before

After

Technical Details

civibot bot commented Jul 17, 2023

colemanw Jul 17, 2023

Choose a reason for hiding this comment

colemanw Jul 21, 2023

Choose a reason for hiding this comment

totten Jul 21, 2023

Choose a reason for hiding this comment

totten commented Jul 25, 2023 • edited Loading

christianwach left a comment

Choose a reason for hiding this comment

christianwach Jul 25, 2023

Choose a reason for hiding this comment

totten Jul 25, 2023

Choose a reason for hiding this comment

totten Jul 26, 2023

Choose a reason for hiding this comment

christianwach Aug 23, 2023

Choose a reason for hiding this comment

christianwach commented Jul 25, 2023

totten commented Jul 26, 2023 • edited Loading

colemanw commented Sep 6, 2023

totten commented Jul 17, 2023 •

edited

Loading

totten commented Jul 25, 2023 •

edited

Loading

totten commented Jul 26, 2023 •

edited

Loading