Reduce use of Globals

[simonwheatley]

Replace all things like this:

function bbl_get_current_interface_lang_code() {
    global $bbl_locale;
    return $bbl_locale->get_interface_lang();
}

With something like this:

function bbl_get_current_interface_lang_code() {
    $bbl_locale = bbl_locale::get_instance();
    return $bbl_locale->get_interface_lang();
}

Also removing the globals and instantiation below each class.

Also consider degloballing the $bbl_translating global, and any others, somehow.

[rmccue]

This seems like a bad idea. This takes the existing, easily-altered global state and hides it inside a static method for no real gain. The global state still exists, it's just now hidden in a place that's harder to access.

For testing purposes, you're typically better off leaving it as a global. This can then easily be swapped out without needing to make all sorts of replacement methods.

[nb]

I agree with @rmccue, singletons are rather inflexible and hard to test. If you are tired of typing global $harharhar, in GlotPress I used a registry-style class, where all those global singletons were static variables and it worked well for me:

<?php
class GP {
    // models
    public static $project;
    public static $user;
    public static $translation_set;
    public static $permission;
    public static $validator_permission;
    public static $translation;
    public static $original;
    public static $glossary;
    public static $glossary_entry;

    // other singletons
    public static $router;
    public static $redirect_notices = array();
    public static $translation_warnings;
    public static $builtin_translation_warnings;
    public static $current_route = null;
    public static $formats;
    // plugins can use this space
    public static $vars = array();
    // for plugin singletons
    public static $plugins;
}

GP::$plugins = new stdClass();

[simonwheatley]

Thanks @nb and @rmccue, you are persuading me. :smile:

If you are tired of typing global $harharhar, in GlotPress I used a registry-style class, where all those global singletons were static variables and it worked well for me:

@nb I'm trying to follow this. Is this a stylistic change from using global, i.e. effectively the same, or is there another advantage?

[nb]

This way the list of global is namespaced and you can have a central place, which lists them all, it’s easier to add extra structure if you want. The $plugins is a good example, where in GlotPress each plugin is encouraged to use their slug as key and it makes accessing other plugins at runtime a bit easier.

[simonwheatley]

@rmccue Any thoughts?

[rmccue]

I agree with @nb here. :)

You still have global state, but it's namespaced, so that's a benefit in terms of global namespace pollution. You might want to look at whether you need all the global state though; GlotPress needs a lot because it's the application itself, but you can probably get away with significantly less.

For example, you could have data that assembles itself when needed. A typical example of this is having a function that returns a filtered array, and hence is generated every time you call it. This is a memory/CPU tradeoff, so it depends on the use case, but can often be a decent way of removing global state.

[simonwheatley]

Well colour me convinced then. Thanks, both. :smiley:

[simonwheatley]

Discussed with @johnbillion and agreed to:

• [ ] Remove globals, replacing with a registry-style class, ala @nb's suggestion: https://github.com/Automattic/babble/issues/220#issuecomment-109289613
• [ ] Consider a backwards compatibility plugin to reintroduce globals if they are required anywhere

[johnbillion]

Anything blocking this?

[joehoyle]

@simonwheatley assigned over to you to merge #312 which should mean we can then close this out.