-
Notifications
You must be signed in to change notification settings - Fork 394
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
Replace includes.php $data by (stable) global variable equivalent #3408
Conversation
@rmuit - thank you for submitting this PR! could you please remove the internal ticket number DG-20990 and PF-1795 when you get a chance? |
Removed. |
@rmuit thank you so much for this! am i reading you correctly that this will only work with ACSF version 2.52? perhaps we should update composer to force that version or higher as part of this PR as well if so? |
Nope. I should have said this more clearly, but "since $GLOBALS['gardens_site_settings'] has 'always' existed,"
|
oh awesome. glad i asked then! thanks |
this definitely fixes the missing "$data" warnings I was seeing recently in factory hook output. so that's a good sign. |
Fixes #3362
Changes proposed:
In factory-hooks/post-sites-php/includes.php, change the usage of $data['gardens_site_settings'] into $GLOBALS['gardens_site_settings'].
At the same time, remove the PHPCS indicator that was only added because $data was used in this file.
Reasons:
Please note: since $GLOBALS['gardens_site_settings'] has 'always' existed, this should be backportable without any issue to every 9.x version. (This includes.php was introduced in an early alpha version of 9.x)
Steps to replicate the issue:
Can be found in a.o. [ internal tickets, known by lcatlett]. I hope you will forgive me for not having exact steps and not even reproducing the error.
But note that the issue means that includes.php does not define global $_acsf_site_name - I don't know BLT but from my viewpoint that can have any kind of effects.
Steps to verify the solution: