dev/core#1651 and dev/core#1637 - Inline editing not working on admin screens and other js packages issues

[demeritcowboy]

Alternate to #16779

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[kcristiano]

@demeritcowboy if I apply this PR and #16779 I get failures on inline editing (Financial Types) as well as adding images in the traditional mailer.

This PR seems to change the URL to http://wpcvrc.test/[civicrm.packages]/kcfinder/browse.php?cms=civicrm&type=images&CKEditor=crmUiId_1&CKEditorFuncNum=0&langCode=en

With just #16779 it works fine

[demeritcowboy]

You would only apply one PR. This is an alternate to 16779 - I think this is better, but either one, alone, work.

[demeritcowboy]

Or maybe on wordpress the /. does something? (in the first changed line)

[kcristiano]

Or maybe on wordpress the /. does something?

It should not.

I see I applied both thinking we needed to fix i10n.js

I'll try with just this PR

[kcristiano]

I rebuilt D7 and WP with just this PR and I consistently get failures as noted above.

#16779 does work properly. I'd recommend merging #16779

[demeritcowboy]

Weird. I was sure I tested it. I might have done it on master and maybe different there. Thanks.

[seamuslee001]

closing in favour of #16779

[demeritcowboy]

Cool. I was curious - I must have had some caching or something. So this is in master:

# No slash, no dot
cv ev "echo Civi::paths()->getUrl('[civicrm.packages]');"
# doesn't recognize it
http://site/[civicrm.packages]

# With a slash
cv ev "echo Civi::paths()->getUrl('[civicrm.packages]/');"
# Result has a slash
http://site/sites/all/modules/civicrm/packages/

# With a slash and dot
cv ev "echo Civi::paths()->getUrl('[civicrm.packages]/.');"
# result is no slash
http://site/sites/all/modules/civicrm/packages

So it's possible to make it work but could look at doing that in master.

[totten]

@demeritcowboy Yup, that's what I see too.

Short version: IMHO, it's a flaw in getPath() / getUrl(). We should patch them to support bare-variables and expand Civi\Core\PathsTest to include that test-case (in addition the existing ones). Agree that should be fixed in master.

Long version: I've filed it as a separate issue https://lab.civicrm.org/dev/core/issues/1653

[totten]

Stylistically, I like this patch better than #16779, but yeah - as you found, the [civicrm.packages] doesn't evaluate as expected.

Since the output before was expected to end in a trailing slash, I can't help thinking that this might have been a one-character patch - just changing /. to /.

[demeritcowboy]

Yeah it's unfortunate I had some local issue/operator error that made it seem like it worked at first. I think for the regression though at this point it was more about not making everyone test/review things again on all cms's.