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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue tags: -revisit before release +revisit before release candidate
webchick’s picture

Issue tags: -revisit before release candidate

Talked 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

webchick’s picture

Issue summary: View changes
Issue tags: +Novice

And actually, this is something that could make a good Novice task. Updated the issue summary with instructions.

webchick’s picture

Issue summary: View changes
jhodgdon’s picture

Novice 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!

kekkis’s picture

Assigned: Unassigned » kekkis

I will work on this as part of the Drupalcamp Baltics sprint, today.

kekkis’s picture

Assigned: kekkis » Unassigned
FileSize
424 bytes

Here 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:

$ grep -r '\$GLOBALS' . | grep -vi 'core/test/' | grep -vi 'core/vendor/' |grep -vi 'core/scripts/' >globals-foo.txt
$ grep -r 'global \$' . | grep -vi 'core/test/' | grep -vi 'core/vendor/' |grep -vi 'core/scripts/' >global-foo.txt

Then I played around with Sublime's multiple select and sort -u and cut to produce the list, finally combining the two files into one.

JeroenT’s picture

Alrighty,

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):

__PHPUNIT_BOOTSTRAP
base_insecure_url
base_secure_url
config
config_directories
config_test_run_module_overrides
destroy-me
efq_test_metadata
entity_crud_hook_test
entity_test_throw_exception
hacked
hook_cache_flush
hook_config_import
hook_config_test
install_state
it_is_pirate_day
larry_test
loader
namespaces
theme_test_output

Removed globals (Only used in Drupal 7):

base_theme_info
channel
cookie_domain
databases
element
forum_topic_list_header
image
installed_profile
is_https
item
items
language
language_content
language_url
menu_admin
multibyte
tag
theme
theme_engine
theme_info
theme_key
theme_path
timers
update_free_access
user

Old globals (Used in Drupal 7 and Drupal 8)

base_path
base_root
base_url
conf
pager_limits
pager_page_array
pager_total
pager_total_items
jhodgdon’s picture

Status: Active » Needs work

Thanks! 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.

kekkis’s picture

And 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.)

jhodgdon’s picture

Yes, 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.

kekkis’s picture

Assigned: Unassigned » kekkis

New try then!

kekkis’s picture

Status: Needs work » Needs review
FileSize
168 bytes

I 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 and it_is_pirate_day aren't actually names for globals in the running core. :)

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Active

Great, 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.

sdstyles’s picture

Status: Active » Needs review
FileSize
2.1 KB

Added core/global.api.php file with all global variables from #13 except $conf which was replaced with $config in D8.

jhodgdon’s picture

Status: Needs review » Needs work

Great, thanks! This is nearly perfect. A few things to fix:

  1. $conf is missing.
  2. +++ b/core/globals.api.php
    @@ -0,0 +1,96 @@
    +/**
    + * Array of persistent configuration variables.
    + */
    +global $config;
    +
    

    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?

  3. +++ b/core/globals.api.php
    @@ -0,0 +1,96 @@
    +global $pager_page_array;
    

    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.

sdstyles’s picture

I'm not sure about the $conf, is it the same as in D7? I found only 3 comments mentions in settings.php

sdstyles’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
514 bytes

The last submitted patch, 15: 2292107-15.patch, failed testing.

jhodgdon’s picture

Status: Needs review » Needs work

Weird. 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:

$GLOBALS['conf']['container_service_providers']

This allows site-specific service provider classes to be defined for the Drupal kernel. So, how to document this... Maybe something like this:

/**
 * Allows defining of site-specific service providers for the Drupal kernel.
 *
 * To define a site-specific service provider class, use code like this:
 * @code
 * $GLOBALS['conf']['container_service_providers']['MyClassName'] = 'Drupal\My\Namespace\MyClassName';
 * @endcode
 *
 * See \Drupal\Core\DrupalKernel::$serviceProviderClasses
 */
global $conf;

The rest of the patch looks great, thanks!

Berdir’s picture

Yeah, $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?

jhodgdon’s picture

The 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.

sdstyles’s picture

Updated docblock for $conf, thank you.

sdstyles’s picture

Status: Needs work » Needs review
JeroenT’s picture

Status: Needs review » Needs work
+++ b/core/globals.api.php
@@ -0,0 +1,110 @@
+ * @code
+ * $GLOBALS['conf']['container_service_providers']['MyClassName'] = 'Drupal\My\Namespace\MyClassName';
+ * @endcode

The code between @code and @endcode should be indented with 2 spaces.

Other than that patch looks good!

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Actually it doesn't have to be indented. In this case, the line is pretty long so I think it's probably better not to.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: 2292107-23.patch, failed testing.

JeroenT queued 23: 2292107-23.patch for re-testing.

sdstyles’s picture

Status: Needs work » Needs review
JeroenT’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as per # 26.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: 2292107-23.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

jhodgdon queued 23: 2292107-23.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: 2292107-23.patch, failed testing.

jhodgdon queued 23: 2292107-23.patch for re-testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Related issues: +#2183323: Replace $GLOBALS['conf']['container_service_providers'] in DrupalKernel with Settings

#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!

  • alexpott committed e71fcdc on 8.0.x
    Issue #2292107 by sdstyles, kekkis, jhodgdon, JeroenT: Clean up globals...
jhodgdon’s picture

I just also removed the globals.php file from the 8.x branch of the Documentation project repository.

Thanks everyone who helped in this effort!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.