Problem/Motivation
On api.drupal.org we have a page listing global variables:
https://api.drupal.org/api/drupal/developer!globals.php/8
This page has traditionally been in the Documentation repository.
For Drupal 8, I think we should move it into core/globals.api.php.
We also need to go through the list of variables there and remove the ones that are no longer part of Drupal 8.
Here are some greps to find the global variables, as of August 26, 2015, which ignore both the vendor and tests directories (you can also ignore core/scripts):
global $foo: https://gist.github.com/webchick/073666ea842c2a2fa60a
$GLOBALS['foo']: https://gist.github.com/webchick/6fb784c3b450d9a1e2e7
Proposed resolution
Fix up the list for Drupal 8.
Remaining tasks
a) Compile a list of existing global variables (outside of Vendor classes and tests) in Drupal 8. This was done on comment #13.
b) Make a patch to document these in Drupal Core:
- Copy the existing globals.php file from the Documentation project to directory core in Drupal Core project.
- Remove globals from that file that no longer exist in Drupal 8.
- Add documentation for the new globals for Drupal 8.
c) Remove the existing globals.php file from the Documentation project for Drupal 8.
[once we have this patch committed, someone else with access to the Documentation project can do this.]
User interface changes
None.
API changes
None.
Data model changes
Task is to gather a list of global variables from that list, compare these against https://api.drupal.org/api/drupal/developer!globals.php/8 and determine which ones are new/outdated, and make a patch.
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-18-23.txt | 571 bytes | sdstyles |
#23 | 2292107-23.patch | 2.55 KB | sdstyles |
#18 | interdiff-15-18.txt | 514 bytes | sdstyles |
#18 | 2292107-18.patch | 2.26 KB | sdstyles |
#15 | 2292107-15.patch | 2.1 KB | sdstyles |
Comments
Comment #1
xjmComment #2
webchickTalked with Jennifer and we both agreed that this is something that could happen at any time, including during RC, so does not need to block RC1. Removing tag.
In the meanwhile, we did revisit this, and despite all efforts to obliterate global / $GLOBALS from core, we still do have some:
global $foo: https://gist.github.com/webchick/073666ea842c2a2fa60a
$GLOBALS['foo']: https://gist.github.com/webchick/6fb784c3b450d9a1e2e7
Comment #3
webchickAnd actually, this is something that could make a good Novice task. Updated the issue summary with instructions.
Comment #4
webchickComment #5
jhodgdonNovice contributor: The first task that would be great to do would be to make a list of what the top-level global variables are from those two "gist" links in the issue summary. Then we can worry about a patch to actually document them. Thanks!
Comment #6
kekkisI will work on this as part of the Drupalcamp Baltics sprint, today.
Comment #7
kekkisHere is the list of all the variable names from
global $foo
and$GLOBALS['foo']
declarations, combined, and sorted. I will let you more seasoned contributors take it from here.I grepped the version 4557d23d7fa6ea0c07a706939820b18425c56470 of 8.0.x, doing:
Then I played around with Sublime's multiple select and
sort -u
andcut
to produce the list, finally combining the two files into one.Comment #8
JeroenTAlrighty,
I used this website (http://jura.wi.mit.edu/bioc/tools/compare.php) to compare the list on https://api.drupal.org/api/drupal/developer!globals.php/8 and the list provided by Kekkis.
This was the result:
New globals (Only used in Drupal 8):
Removed globals (Only used in Drupal 7):
Old globals (Used in Drupal 7 and Drupal 8)
Comment #9
jhodgdonThanks! But.... I took a quick look at the list in #7. I think it needs another look. There are several globals in the .txt file that I do not see anywhere in the two gist files.
Oh. I think that the problem is that the list in #7 still includes globals from tests. The grep commands there did not exclude all test files, only core/test. We need to take those out.
Comment #10
kekkisAnd there I was thinking that I could actually improve the grep (by removing 'false' hits by excluding a smaller set of files using the core/ prefix).
If I use the version laid out in the gist,
grep -r 'global \$' . | grep -vi test | grep -vi vendor
, and maybe add|grep -vi 'core/scripts'
, is that a good enough starting point?Edit. (Maybe I just need to try! Well, I'll wait for a reply before acting.)
Comment #11
jhodgdonYes, we did a grep -vi test to exclude anything with test or Test in the name. And those other grep pipes look right to me too.
Comment #12
kekkisNew try then!
Comment #13
kekkisI now composed a new list using the same means I did earlier, only using different exclusions (the ones laid out in #10 and #11. The list became significantly shorter, and I am relieved to see that
destroy-me
,larry_test
andit_is_pirate_day
aren't actually names for globals in the running core. :)Comment #14
jhodgdonGreat, thanks! That looks like the right list of globals.
Now all we need to do is document them. And I agree that we should not have pirate day in Drupal Core, as much as it might be fun!
Now that we have this list, I am setting the issue to Active to reflect that we don't have a patch yet.
So. A starting point for the patch would be to take the Drupal 7 globals.php file (which is in the Documentation project repository by the way, not Drupal Core), and copy it to directory core. Then remove all the globals that no longer exist in Drupal 8, and add entries for the new variables for Drupal 8.
If whoever decides to do this patch does not know how to document the new variables, you could just make empty documentation blocks for them, and we can go from there.
Updated summary with current status.
Comment #15
sdstyles CreditAttribution: sdstyles at FFW commentedAdded core/global.api.php file with all global variables from #13 except $conf which was replaced with $config in D8.
Comment #16
jhodgdonGreat, thanks! This is nearly perfect. A few things to fix:
Actually, looking at how this is generated and used, I believe that the global $config is actually an array of configuration overrides from the settings.php file, not the array of config variables, right?
This doc block doesn't have the "The array index is..." line that the other pager variables have. Maybe it should?
The rest looks fine to me.
Comment #17
sdstyles CreditAttribution: sdstyles at FFW commentedI'm not sure about the $conf, is it the same as in D7? I found only 3 comments mentions in settings.php
Comment #18
sdstyles CreditAttribution: sdstyles at FFW commentedComment #20
jhodgdonWeird. No, it doesn't look like $conf is the array of config in Drupal 8. Instead, it looks like it is an array, and the only array element that is ever looked at or used is:
This allows site-specific service provider classes to be defined for the Drupal kernel. So, how to document this... Maybe something like this:
The rest of the patch looks great, thanks!
Comment #21
BerdirYeah, $conf should have been removed but that remaining usage was overlooked when converting things away from it.
I'm not even sure we should document it at all, at least not as a global, just as php storage configuration, if we have that. It's definitely nothing you should ever see or use outside of settings.php. I'd instead just add some specific documentation about that in settings.php as we seem to not have that yet. Probably in a separate issue?
Comment #22
jhodgdonThe purpose of having the globals.php is so that these variables can be looked up on api.drupal.org. If we add documentation in settings.php, this doesn't happen. So I would prefer to have it documented in the globals.php file so that we at least are certain to have it documented, however obscure it may be.
Comment #23
sdstyles CreditAttribution: sdstyles at FFW commentedUpdated docblock for $conf, thank you.
Comment #24
sdstyles CreditAttribution: sdstyles at FFW commentedComment #25
JeroenTThe code between @code and @endcode should be indented with 2 spaces.
Other than that patch looks good!
Comment #26
jhodgdonActually it doesn't have to be indented. In this case, the line is pretty long so I think it's probably better not to.
Comment #29
sdstyles CreditAttribution: sdstyles at FFW commentedComment #30
JeroenTRTBC as per # 26.
Comment #32
JeroenTComment #33
jhodgdonComment #37
jhodgdonComment #38
alexpott#2183323: Replace $GLOBALS['conf']['container_service_providers'] in DrupalKernel with Settings is the issue to rid us of
$conf
- i ponder if it can or should be done now. I guess not.I'm with @jhodgdon - if we have it we should document it.
Committed e71fcdc and pushed to 8.0.x. Thanks!
Comment #40
jhodgdonI just also removed the globals.php file from the 8.x branch of the Documentation project repository.
Thanks everyone who helped in this effort!