Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(REF) dev/core#1637, dev/core#1651 - Restore format of packagesBase #16791

Merged
merged 3 commits into from
Mar 16, 2020

Conversation

totten
Copy link
Member

@totten totten commented Mar 16, 2020

Overview

This is an alternative to #16779 / #16780. I think #16780 is better/simpler, but it had a defect, which is addressed here - and ultimately leads to the smallest/simplest patch.

Before and After

  • In 5.22.x, there's a resourceBase but no packagesBase.
  • In 5.23.0, there's both resourceBase and packagesBase, and they both end in /.
  • In 5.23.1, resourceBase and packagesBase diverge -- packagesBase accidentally loses its /. Consumers break.
  • In PR 16779, resourceBase and packagesBase remain diverged -- and every consumer of packagesBase is made defensive.
  • In PR 16780, resourceBase and packagesBase are once again aligned (both ending in /)... but packagesBase is miscomputed.
  • In this PR, resourceBase and packagesBase are once again aligned (both ending in /).

Comments

  • This is fundamentally a simple one-character fix (cc8f6f1) for dev/core#1651, but the PR looks bigger because of the revert (1ff4b7b) and some small docblocks (605ee23).
  • One might suggest shipping with dev/core#1651 and dev/core#1637 - Inline editing not working on many admin screens and other js packages issues #16779 because it's been reviewed -- and deferring cleanup to master (5.25 or 5.26). The problem is that this will lock-in the ambiguity in the content of packagesBase: with any new code (core or contrib) which references packagesBase, they'll either assume that there is no / or they'll have to reproduce the defensive snippets. The former means we can't bring resourceBase and packagesBase back into alignment (without a break), and the latter spreads the mess around.

totten added 3 commits March 15, 2020 17:20
These use-cases had been tested during PR dev for 5.23.alpha, but they
regressed in 5.23.1.  In 5.23.1's civicrm#16735, note item (5) and the flip-flop on
`/.` Item (5) references some greps to find references `/.` For obscure
reasons, the  file `l10n.js.tpl` didn't match the greps.
Try to prevent future bounciness in changing these variables.
@civibot
Copy link

civibot bot commented Mar 16, 2020

(Standard links)

@civibot civibot bot added the 5.24 label Mar 16, 2020
@totten
Copy link
Member Author

totten commented Mar 16, 2020

FWIW, I did some local r-run on D7 using the edit-in-place functionality and confirmed (a) the bugginess before patching, (b) the fixiness of 16779, and (c) the fixiness of this PR.

@demeritcowboy
Copy link
Contributor

Tested inline edit and mailing issue on drupal 7 - 5.23.2 / 5.24 / master. All good.

Also unless there's some weird use of eval or something there are no other references to CRM.config.packagesBase other than those in 3130950

@totten
Copy link
Member Author

totten commented Mar 16, 2020

Oh, yeah, good idea checking for more references.

Also, FWIW, the reason the l10n.js.tpl slipped through in 5.23.1... I had done a grep in universe for ]/. to find things like this. The grep had a bunch of false-matches on minified JS files, so I ignored all files with \.js ... but then it skipped l10n.js.tpl... Trying another search, I'm also unable to find additional examples.

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Mar 16, 2020

Maybe we could use something like an emoji as the delimiter instead of [ ]. Or chr(1). It has the extra bonus of being fun to watch people try to type in...

Oh the internet tells me emojis are valid filename chars. Probably shouldn't tell too many people that...guess what will happen...

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

Successfully merging this pull request may close these issues.

3 participants