Fix false negatives in checkResourceUrl() #12460
Conversation
|
Can one of the admins verify this patch? |
|
(Standard links)
|
|
This check was my bad idea, so I'll review this. Also could someone with permissions add Greg to the whitelist? This is his first commit to core (thank you Greg!) but is a long-time contributor in CiviCRM chat. |
|
Jenkins add to whitelist |
|
@freephile Greg can you fix the style issue pls https://test.civicrm.org/job/CiviCRM-Core-PR/21553/checkstyleResult/new/ |
|
(style issue will be that drupal coding standards -which we follow - require upper case FALSE ) |
There was a problem hiding this comment.
I'm a bit mystified - The string being checked isn't the resource URL, but the HTTP response, which should be "HTTP/1.1 200 OK". It sounds like yours just returns "200 OK" - but when I load the URL myself, I see the response as "HTTP/1.1 200 OK".
Regardless, I agree that this change is safe and expresses the original intent more accurately, so I'm giving this "merge-on-pass".
|
I've added merge-on-pass per @MegaphoneJon review - however, it won't pass until the style warning is fixed |
Without this proposed fix, I'm receiving false errors about the CiviCRM Resource URL not being set correctly. The stripos() function will return '0' if the needle string is the first thing found in the haystack. Thus, you must use equivalence rather than a true/false test where "position zero" evaluates to false. Tested on Civi 5.3.0 on WordPress 4.9.7 I can access http://coastaltrails.org/wp-content/plugins/civicrm/civicrm/packages/jquery/css/images/arrow.png But WITHOUT the fix, I get an error ``` The Resource URL is not set correctly. Please set the CiviCRM Resource URL. ``` I have CiviCRM Resource URL set to `[civicrm.root]/` My URL Variables are calculated as: ``` [cms.root] http://coastaltrails.org/ [civicrm.root] http://coastaltrails.org/wp-content/plugins/civicrm/civicrm/ [civicrm.files] http://coastaltrails.org/wp-content/uploads/civicrm/ These variables are computed automatically using civicrm.settings.php and its options, such as CIVICRM_TEMPLATE_COMPILEDIR. ```
|
I've updated this PR to fix the style issue. Can be merged when tests complete. |
Overview
Without this proposed fix, I'm receiving false errors about the CiviCRM Resource URL not being set correctly.
Before
I can access http://coastaltrails.org/wp-content/plugins/civicrm/civicrm/packages/jquery/css/images/arrow.png But WITHOUT the fix, I get an error
I have CiviCRM Resource URL set to
[civicrm.root]/My URL Variables are calculated as:
After
No Error on CiviCRM System Status page. Footer 'system status' is not Red
Technical Details
The stripos() function will return '0' if the needle string is the first thing found in the haystack. Thus, you must use equivalence rather than a true/false test where "position zero" evaluates to false.
Comments
Tested on Civi 5.3.0 on WordPress 4.9.7