VUFIND-1680 / phpCas 1.6 update #3550
Conversation
Add phpCas/service_base_url parameter Change phpCas logging see : https://openlibraryfoundation.atlassian.net/browse/VUFIND-1680
Add phpCas/service_base_url parameter Change phpCas logging see : https://openlibraryfoundation.atlassian.net/browse/VUFIND-1680
There was a problem hiding this comment.
Thanks, @mathieugrimault! See below for a few suggestions. Also note that there's a small style issue you can automatically fix by running composer fix and committing the resulting changes.
| } elseif (isset($this->getConfig()->Site->url)) { | ||
| // fallback method | ||
| $service_base_url = [ | ||
| parse_url($this->getConfig()->Site->url, PHP_URL_SCHEME) . '://' . |
There was a problem hiding this comment.
Is it worth calling parse_url just once and then processing the array it returns? Seems like that might be more efficient. Also, do we need to assemble this a little more carefully (e.g. checking whether some components are missing)? In particular, if there is a port number, do we need to add a ':' in front of it? I think parse_url just returns an integer in that situation.
There was a problem hiding this comment.
Another possibility might be to use a regular expression to extract the beginning of the URL, if that's easier than messing with parse_url.
Ok with vendor/bin/phing php-cs-fixer |
There was a problem hiding this comment.
Thanks again for the work on this, @mathieugrimault! Since I can't test this for real, I decided to write some unit tests to cover various scenarios in configuration processing. This revealed a few minor bugs. I have refactored the code to make testing easier, and I have fixed the bugs (mainly related to missing elements in the parse_url return array) in the refactored code.
Can you please test this revised version and make sure it works correctly in your real-world scenario? Once you confirm that it is working (and if you approve of my changes), please let me know and I can merge the PR.
|
Thanks Demain ! |
Add phpCas/service_base_url parameter
Change phpCas logging
see : https://openlibraryfoundation.atlassian.net/browse/VUFIND-1680