Make WordPress Core

Opened 8 years ago

Last modified 3 years ago

#37699 new enhancement

Death to Globals Episode #1: A Registry, A Pattern

Reported by: wonderboymusic's profile wonderboymusic Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

Storing application state in globals is ... bad.
Using global state to store objects is ... bad.
Using globals to avoid writing classes is ... bad.
Using globals to pass data between functions is ... bad.
Training people to assume that globals will always be set to the value they expect is ... bad.

Globals are an artifact of a #dark-er time, think PHP 3. It would be nice if we didn't use globals. We have a backward compatibility strait jacket, but that shouldn't stop us from exploring ways to eradicate them.

We can start with class instances, because variables hold a reference to the actual instance when set. We can also start with a simple registry that allows us to create a data that can get / set globals for us (or not!). We can also add a few static methods onto WP to hide all of this away.

Attached is a POC that removes (every instance of?) $wpdb as a global, and instead uses the registry. Take a glance. All unit tests (seem to) pass.

This will evolve before it becomes a reality, but I encourage you to unleash your imagination unto a world where WordPress' codecase resembles something globallessly lovely.

Attachments (11)

37699.diff (216.8 KB) - added by wonderboymusic 8 years ago.
37699-composition.diff (62.5 KB) - added by wonderboymusic 8 years ago.
37699-query.diff (26.5 KB) - added by wonderboymusic 8 years ago.
37699-images.diff (7.2 KB) - added by wonderboymusic 8 years ago.
37699-hasher.diff (6.3 KB) - added by wonderboymusic 8 years ago.
37699.2.diff (4.3 KB) - added by wonderboymusic 8 years ago.
37699-version.diff (45.6 KB) - added by wonderboymusic 8 years ago.
37699-version.2.diff (47.4 KB) - added by wonderboymusic 8 years ago.
37699-get_current_blog_id.diff (9.6 KB) - added by wonderboymusic 8 years ago.
37699-version.3.diff (10.9 KB) - added by wonderboymusic 8 years ago.
37699-version.4.diff (27.2 KB) - added by wonderboymusic 8 years ago.

Download all attachments as: .zip

Change History (128)

@wonderboymusic
8 years ago

#1 follow-up: @schlessera
8 years ago

Hell yes, it would be awesome to get rid of the globals.

I'm fully aware that this is just a very rough first pass to get the discussions started, so I won't delve too much into technical details. I'd like to share some early observations, though, based on what you posted so far.

1. Patch is huge

The patch is very unwieldy when the goal is to discuss basic concepts first. I think it would be more practical to limit patches on the actual "Registry", and one or two use cases until the general direction is clear.

2. Existing pattern

What you call "Registry" here is commonly called a "Service Locator", which is a known pattern. The Service Locator is responsible for letting major subsystems interact with each other, by providing instances to each subsystem when requested.

3. Still global

The way you've implemented this Registry/Service Locator here means that you've replaced one global with another global. When we start to put more subsystems into that Service Locator, the number of globals will of course reduce, but it cannot be completely brought down this way. A reference to WP::get() means that the method get() is called on the static (=global) WP "instance". I put "instance" in brackets here, because it has not technically been instantiated (through the new keyword), but it has state, and this state is made globally accessible.

4. Using WP as object

Relying on the WP class name as the Service Locator is not ideal, as a future version of WordPress (PHP 5.3+) would most likely have WP as the root namespace. That future version would probably offer something like WP\Service::get( 'wpdb' );, so something like WP_Service::get( 'wpdb' ); would be preferable. Keeping this as future-proof and flexible as possible should be a priority.

5. Dependency injection

WordPress is mostly procedural, but the places in the code where we do already have classes should try to start using dependency injection.
As an example, consider the WP_Comment_Query class, for which you proposed something like the following:

class WP_Comment_Query {

   protected $dbh;

   public function __construct( $query = '' ) {
      $this->dbh = WP::get( 'wpdb' );
      // [...]
   }
}

There's a missed opportunity there to start using dependency injection to make it easier to do unit tests. Even worse: right now, with the globals, the unit tests can still set the global to a mock DB. With the above code, there's no proper way of injecting a mock DB for testing anymore.

So, while we cannot yet have a proper injector decide what to inject in what context, we can at least let the constructor take an injected dependency, and provide a "Poka-yoke" for as long as we are not able to do real injection. This would look something like this:

class WP_Comment_Query {

   protected $dbh;

   public function __construct( $query = '', $wpdb = null ) {
      $this->dbh = null !== $wpdb ? $wpdb : WP_service::get( 'wpdb' );
      // [...]
   }
}

Improvements would be using the short-form ternary (PHP 5.3+) and typehinting against an interface. However, please, do not typehint against the WPDB class, this defeats the whole point. This would just very tightly couple the class to the exact WPDB implementation, and any dependency injection would just be of esthetic nature.

With the above constructor, the injected dependency is optional, so existing code will still work, but the unit tests can now inject a mocked WPDB instance into the constructor for real unit tests.

6. What about the interface

As mentioned under 5., typehinting against an interface would be a clear improvement. Let's imagine we had a WPDB_Interface and examine what would happen if we use it together with the idea of a Service Locator.

First of all, any code making DB operations would need to be coded against that interface, not against the actual implementation. For code that does not using any typehinting or instance-checking, this should not make a difference.

For our example code above, we would now have something like this:

class WP_Comment_Query {

   /** @var WPDB_Interface */
   protected $dbh;

   public function __construct( $query = '', WPDB_Interface $wpdb = null ) {
      $this->dbh = null !== $wpdb ? $wpdb : WP_Service::get( WPDB_Interface );
      // [...]
   }
}

We have two changes here:

  1. The constructor argument is type-hinted. This means that, would we instantiate the WP_Comment_Query through a real Injector, the Injector could by itself figure out that it needs to pass the instance of the current DB handler into that constructor. So in that (wishful thinking) scenario, neither the WP_Comment_Query class, nor its surrounding code would need to know anything about the DB stack at all. WP_Comment_Query needs something that behaves like a WPDB, and it can just assume that when it gets instantiated, what it needs is automagically available within its constructor.
  2. The poka-yoke fallback that uses the Service Locator does not request an arbitrary string identifier, it requests an object that implements the interface it needs. The Service Locator, which should know more about the current environment than the WP_Comment_Query class, will then decide what exact object to pass around. Some objects, like the WPDB here, are globally shared, so everyone gets a reference to the same instance. Others might be freshly instantiated for each request, like a WP_Query. The Service Locator will deal with this behind the scenes, the ones requesting such interfaces should not need to care.

Hope this all makes sense, and can't wait to see where this ticket will take us... :)

#2 @schlessera
8 years ago

Now that I think about it, I might have just made some assumptions regarding the pattern name (have been doing too much work in that area lately). What you propose can indeed be called a Registry, provided that it never instantiates an object by itself, but only stores them. This would mean that the responsibility for instantiating the objects must lie somewhere else though.

Opposed to that, the Service Locator would not retrieve instances to store them, but would rather instantiate them as needed when they are requested (and then storing them if they need to be shared).

#3 @schlessera
8 years ago

I just thought of another argument for introducing interfaces.

Right now, we can't just have everything within the Registry/Service Locator, as there's still lots of third-party code that depends on the $wpdb global.

Having interfaces also allows us to use the Decorator pattern, which we could use to elegantly show "deprecated" notices for code that still uses the actual global. So, the WPDB object gets instantiated, the Registry saves its instance, and then a DeprecatedWPDBDecorator gets wrapped around that instance, and this wrapped version gets stored as the global $wpdb. The decorator would just accept the exact same methods as the normal WPDB, but for each method, it would throw a deprecated notice before forwarding the arguments to the real method.

This would open up a safe migration path to get people away from using the globals with due notice.

#4 follow-up: @schlessera
8 years ago

Sorry for writing multiple messages, but every time I click send, a new thought crosses my mind.

When evaluating whether interfaces are feasible, it should be noted that the current class name (wpdb) would be ideal as the first level of interface migration.

So, I would recommend starting by creating an interface wpdb, and rename the current existing concrete implementation to MySQLwpdb implements wpdb. In this way, we have introduced an interface that can be used for typehinting, while not breaking any existing instanceof checks.

Once namespaces are available, I would recommend adding another layer:
interface wpdb;
interface WP\Database extends wpdb;
class WP\MySQLDatabase implements WP\Database;

The WP\MySQLDatabase would still pass the $wpdb instanceof wpdb checks.

#5 @nicholas_io
8 years ago

I'm all for it. but this seems to be a duplicated of #32468

#6 @johnjamesjacoby
8 years ago

The $wp_query vs. $wp_the_query juggle would benefit from this nicely – give every registered global a way to stash & unstash themselves.

#7 @dnaber-de
8 years ago

Interesting ticket and I agree with the explanation of @schlessera. However these considerations touches another aspect of WordPress' code design.

Training people to assume that globals will always be set to the value they expect is ... bad.

That can't be solved by just providing a Repository or implementing dependency injection when using mutable objects. In case of $wpdb all relevant attributes are public writable so you can't trust its state. I know, that can't be solved due to backward compatibility but it should be considered for ongoing development of new components. Otherwise the issue of an unreliable system state can't be solved IMO.


#8 @jdgrimes
8 years ago

Related: #31556

#9 in reply to: ↑ 4 ; follow-ups: @jdgrimes
8 years ago

Replying to schlessera:

When evaluating whether interfaces are feasible, it should be noted that the current class name (wpdb) would be ideal as the first level of interface migration.

So, I would recommend starting by creating an interface wpdb, and rename the current existing concrete implementation to MySQLwpdb implements wpdb. In this way, we have introduced an interface that can be used for typehinting, while not breaking any existing instanceof checks.

This would still break any code that was doing new wpdb() though. Not sure how common that is, but there may be plugins that do it.

#10 follow-up: @wonderboymusic
8 years ago

if the class is loaded (and WordPress loads everything), why wouldn't new wpdb work?

The key things here are:

  • Existing globals have to still exist for back compat, but the instantiation can be hidden
  • remove as many global imports as is possible from functions
  • lazy-load some class instances that aren't expected to always be in global scope ($wp_hasher, por ejemplo)
  • for some keys, perhaps enforce that the proper value is always returned, so that a plugin can't null out its value

one of the things I noticed yesterday when fiddling with the POC - there are a ton of places in core that do:

function get_internet() {
    global $wpdb, $blog_id;
    ...
    $prefix = $wpdb->get_blog_prefix( $blog_id );
}

In instances like this, getting rid of the globals isn't enough, seems like there should be a function that does this for you:

function get_site_prefix( $id = null ) {
    if ( ! $id ) {
        $id = get_current_blog_id();
    }

    $db = WP::get( 'wpdb' );
    return $db->get_blog_prefix( $id );
}

The result is that many of the functions in core are just there as a procedural convenience. Often, we abstract out complex logic from functions to classes and then have functions do as little as possible while wrapping Singletons, or as a convenient way to act on instances in a simple way programmatically.

Anything that results in fewer instances of importing global variables is an upgrade. Anything that abstracts it out so that if global scope disappears, the code will still work, is a vast improvement.

This could happen in baby steps. $wpdb is so mission-critical to the codebase, I used it as an example to see if what I proposed is possible.

#11 in reply to: ↑ 9 @dnaber-de
8 years ago

Replying to jdgrimes:

Replying to schlessera:

When evaluating whether interfaces are feasible, it should be noted that the current class name (wpdb) would be ideal as the first level of interface migration.

So, I would recommend starting by creating an interface wpdb, and rename the current existing concrete implementation to MySQLwpdb implements wpdb. In this way, we have introduced an interface that can be used for typehinting, while not breaking any existing instanceof checks.

This would still break any code that was doing new wpdb() though. Not sure how common that is, but there may be plugins that do it.

Of course these improvements would require preliminary steps to take. Especially using semantic versioning that indicates backward incompatible changes.

#12 in reply to: ↑ 10 @charliespider
8 years ago

Replying to wonderboymusic:

if the class is loaded (and WordPress loads everything), why wouldn't new wpdb work?

I think he was replying to schlessera's comment about changing the wpdb class to be an interface

#13 follow-up: @wonderboymusic
8 years ago

I think protected $dbh or $db is a baby step that could made in the Query classes. Uses composition, strips out a battery of global imports. Yes, the classes are still using "a global," but the methods themselves are using a property

#14 @charliespider
8 years ago

building on schlessera's comments,

I think a better solution to a Registry or Service Locator would be to just do things properly and use a Dependency Injection Container...

BUT...

when replacing globals, do NOT use the DI container directly for obtaining the class you want (like wpdb), because that would basically be service location, which is not a good way to go about things and considered an anti-pattern by some, but instead use a static factory class. Then your factory class, can have access to the DI container, which at least minimizes your use of service location to specific classes as well as minimizing the scope of the service location (since a db connection factory only ever returns a db connection, as opposed to being used for locating any number of services).

Over time, as procedural code gets replaced with OOP, use of these factories can be dropped in favour of injecting a class's required dependencies upon construction.

So the use of factories for specific service location is a stepping stone to proper dependency injection, with the end goal being dependency inversion, and the adoption of modern best practices.

#15 @wonderboymusic
8 years ago

37699-composition.diff uses Composition in the class-wp-*-query classes to reduce global imports of $wpdb by 32.

#16 follow-up: @wonderboymusic
8 years ago

In 38275:

Query: add a protected field, $db, (composition, as it were) to WP_*_Query classes to hold the value for the database abstraction, instead of importing the global $wpdb into every method that uses it. Reduces the number of global imports by 32.

See #37699.

#17 in reply to: ↑ 13 ; follow-up: @charliespider
8 years ago

Replying to wonderboymusic:

instead of :

<?php
// src/wp-includes/class-wp-comment-query.php
    public function __construct( $query = '' ) { 
        this->db = $GLOBALS['wpdb']; 

why not:

<?php
// src/wp-includes/class-wp-comment-query.php
    public function __construct( $query = '', wpdb $wpdb = null ) { 
        this->db = $wpdb instanceof wpdb ? $wpdb : $GLOBALS['wpdb']; 

and then when constructing that class:

<?php
// if we still NEED to grab the global 
global $wpdb
new WP_Comment_Query( $comment_args, $wpdb );
// else use an already injected instance (assuming $this->wpdb === $GLOBALS['wpdb'])
new WP_Comment_Query( $comment_args, $this->wpdb );

this would be a better first step towards proper dependency injection, because eventually, you could remove the reference to $GLOBALS altogether

Last edited 8 years ago by charliespider (previous) (diff)

#18 in reply to: ↑ 17 ; follow-up: @ChriCo
8 years ago

Replying to wonderboymusic:
why not:

<?php
// src/wp-includes/class-wp-comment-query.php
    public function __construct( $query = '', wpdb $wpdb = null ) { 
        this->db = $wpdb instanceof wpdb ? $wpdb : $GLOBALS['wpdb']; 

this was already pointed out by @schlessera . Also you should not add the wpdb-class type-hint. Instead you should implement an interface and use this for Type Hinting (see above why) ...

But yes, the $wpdb = NULL should be added to the constructor.

Last edited 8 years ago by ChriCo (previous) (diff)

#19 in reply to: ↑ 18 @charliespider
8 years ago

Replying to ChriCo:

huh... ya I guess what I suggested is pretty close to the same thing pointed out by @schlessera

and ya I totally agree about the use of an interface

thnx

#20 @wonderboymusic
8 years ago

In 38279:

Query: use composition for $db in WP_Query, removes need to import global $wpdb in multiple methods.

See #37699.

#21 follow-up: @jorbin
8 years ago

@wonderboymusic I wonder if updating the magic methods to exclude wpdb is a more backwards compatible change than changing the unit tests.

#22 @jorbin
8 years ago

[38278] seems to not have gotten mentioned in here. That is the change I am referencing above.

#23 in reply to: ↑ 21 @wonderboymusic
8 years ago

Replying to jorbin:

@wonderboymusic I wonder if updating the magic methods to exclude wpdb is a more backwards compatible change than changing the unit tests.

this is why we pay you the big bucks

#24 @wonderboymusic
8 years ago

In 38280:

Query: use composition for $db in WP_Date_Query, removes need to import global $wpdb in multiple methods.

See #37699.

#25 @tfrommen
8 years ago

Sorry in advance if this may sound rude - it's not meant that way - but ... why is this in trunk already? I mean, the ticket is quite young, and yet there are several things both discussed and suggested that the changesets are lacking.

#26 @wonderboymusic
8 years ago

@tfrommen the point isn't to commit everything into trunk at once, the point is to use less globals in WordPress. The ideal is no global variables, but there is no chance of that happening and maintaining BC. In the meantime, let us imagine what incremental changes we can make that look towards that future.

DI is an idea, not a requirement. Using composition is incremental, not a final solution. If we decide to use a Service Locator, we now only need to update 5 properties, instead of 40 instances within the methods. We could make the decision to use DI, or not, I haven't decided if the extra args are a good idea or not, but I do think composition is an improvement.

We are in Alpha, this is exactly when changes like this go into trunk.

#27 @johnjamesjacoby
8 years ago

@tfrommen The changesets related to this ticket, so far, have been incremental, and are general tidying that could have been done literally at any point in WordPress's life. None of the more complex registration code has gone in yet.

While these changesets may be in preparation for something related to this ticket, if nothing else happens but the code simplifications that have gone in so far, it's still a huge win.

#28 @tfrommen
8 years ago

@wonderboymusic the thing is there isn't actually any composition present. So far, multiple per-method imports from the global scope have been replaced by a one-time global look-up.

#29 @wonderboymusic
8 years ago

In 38303:

Media: add a function, wp_get_additional_image_sizes(), that wraps the retrieval of the global $_wp_additional_image_sizes. Removes 6 global imports.

See #37699.

#30 @wonderboymusic
8 years ago

In 38304:

Login: retrieve_password() does not need to import 2 globals that it does not use.

See #37699.

#31 @Frank Klein
8 years ago

Concerning 37699-hasher.diff, I'd propose to avoid adding more code into wp-includes/functions.php.

Let's use another file that contains only this and similar functions.

#32 follow-up: @wonderboymusic
8 years ago

In 38355:

Site Icon: There is no good reason for class-wp-site-icon.php to drop a global instance of itself whenever the file is loaded. The lone use of the global instance of WP_Site_Icon is in an AJAX action that provides virtually no way to override - the file is loaded immediately before the global is used.

Let us remove the $wp_site_icon global. I will fall on the sword if this comes back to bite us (waiting with bated breath).

See #37699.

#33 in reply to: ↑ 32 @jorbin
8 years ago

Replying to wonderboymusic:

In 38355:

Site Icon: There is no good reason for class-wp-site-icon.php to drop a global instance of itself whenever the file is loaded. The lone use of the global instance of WP_Site_Icon is in an AJAX action that provides virtually no way to override - the file is loaded immediately before the global is used.

Let us remove the $wp_site_icon global. I will fall on the sword if this comes back to bite us (waiting with bated breath).

See #37699.

I asked @ipstenu to search the plugin repo and she couldn't find anything using $wp_site_icon. It would still be a good idea to do a dev-notes post announcing this and any other potentially breaking changes from global removal.

#34 @Ipstenu
8 years ago

Results of WP Plugin Dir Scan on $wp-site-icon: Zero plugins use it.

#35 @wonderboymusic
8 years ago

In 38383:

Unit Tests: after r38303, replace usage of global $_wp_additional_image_sizes with wp_get_additional_image_sizes().

See #37699.

#36 @wonderboymusic
8 years ago

In 38387:

Roles: set a property, $db, on WP_Roles to reduce global imports.

See #37699.

#37 follow-up: @wonderboymusic
8 years ago

In 38388:

Multisite: move get_current_site() to load.php so that it can be used in more places, instead of importing global $current_site.

See #37699.

#38 @wonderboymusic
8 years ago

In 38397:

Press This: the file for the WP_Press_This class should not produce side effects. Similar to what we did in r38355 for WP_Site_Icon, drop the instances of global instantiation for $wp_press_this via loading the file. The variable can be set inline when necessary. In most of those places, if the global is already set, the file does not load and stomp it currently.

See #37699.

#39 @wonderboymusic
8 years ago

In 38398:

Unit Tests:

  • Automatically delete objects that we were created during wpSetUpBeforeClass - posts, comments, terms (except 1), and user (except 1)
  • The amount of leftover data between tests was breathtaking - use the new function: _delete_all_data()
  • Commit database transactions for all TestCases, not just those that implement wpSetUpBeforeClass and wpTearDownAfterClass
  • The tests run 10-20 seconds faster now

See #37699.

#40 in reply to: ↑ 9 @tw2113
8 years ago

Replying to jdgrimes:

This would still break any code that was doing new wpdb() though. Not sure how common that is, but there may be plugins that do it.

I can think of at least one time where I created my own instance for an external read-only database. Been a couple years, but it's still in place last I checked.

#41 @dlh
8 years ago

A report from "the wild": [38279] broke a site I work on. But, there are a lot of steps to replicating what happened, and a plugin is involved. It might be impossible for core to account for a case like it.

  1. This site caches some WP_Query objects as transients. The serialized WP_Query now includes the $db property.
  2. I have Query Monitor enabled, including its QM_DB class.
  3. QM_DB keeps a trace of queries, and any objects in the trace are serialized into the transient, too.
  4. One of the classes on this site tries to prevent you from unserializing it by adding a __wakeup() method with wp_die(). That object was serialized in the trace, and when it was later unserialized via get_transient(), it called wp_die().

I would also note that an exception would have been thrown when setting the transient if a closure had been in the trace: 'Serialization of 'Closure' is not allowed'.

Last edited 8 years ago by dlh (previous) (diff)

#42 @wonderboymusic
8 years ago

In 38455:

List Tables: AJAX actions for List Tables do not need to declare global $wp_list_table. List tables on admin screens are in global scope, and they contain hooks that don't pass the the list table as context, hence using globals there so that functions can import them. That problem does not exist in the AJAX actions, which are virtually impossible to hook into as is.

See #37699.

#43 @jacobsantos
8 years ago

In relation to 37699-version.2.diff, there are several problems that should be addressed.

  1. If $wp_version is not going to change, then it should be a constant.
  2. Registry patterns in OOP are meant to store objects. Using them to store arbitrary data does nothing that global does not already do. It is an hackish approach, but compare https://github.com/MimicCMS/Mimic.Experimental/tree/master/src/Storage and https://github.com/MimicCMS/Mimic.Experimental/blob/master/src/Application.php or a better implementation in https://github.com/thephpleague/container.
  3. Thinking about globals and removing them for the sake of removing them, is not thinking of improving the WordPress architecture. You end up with the mess like WP_Customize_Manager. Refactoring is hard, because you have to think about the relationships between the code and how best to improve the code for the long term. The WP_Container, might be a short term solution, until something better comes along, but the question is when would the better solution be developed and which intermediate steps will be take to incrementally improve the code from Globals to WP_Container to ???. #32468 was meant for safely storing objects with retrieval in the development vein of MimicCMS/Mimic.Experimental.

It seems like this is already the direction WordPress will be going. I would recommend looking at Pimple or The PHP League Container implementation, if you are looking for inspiration for a PHP5.2 implementation. The PHP League seems to be the most basic implementation. There also exists a Zend Framework implementation.

At the very least, I would recommend using an interface, so that the WordPress implementation could be replaced with Pimple or The PHP League Container or another custom solution. You may look at my MimicCMS implementation for an example of how that would work.

As an aside, the experimental code isn't that bad. Doesn't go into the depths that service locator libraries do, but it wasn't meant to go to that level. I always expect my old code to be shit, but I am surprised when it isn't completely terrible. Not sure if that means I'm not learning enough.

#44 @wonderboymusic
8 years ago

WP_Container is just a POC, with Pimple in mind. $wp_version is a bad example, but one that's easy to test. The first goal is to reduce global imports as much as possible. Second is to use some form of registry/container to store instances, while also setting globals to maintain BC - mainly, an abstraction that hides away the global nonsense. I don't feel comfortable with what I have yet.

#45 @wonderboymusic
8 years ago

In 38457:

Multisite: use get_current_blog_id() where applicable, in lieu of plucking the $blog_id global from outer space.

See #37699.

#46 @wonderboymusic
8 years ago

In 38458:

Multisite: use get_current_site() instead of $GLOBALS['current_site'] (stop yelling!) in a few remaining spots.

See #37699.

#47 @wonderboymusic
8 years ago

In 38459:

General: use get_bloginfo( 'version' ) instead of global $wp_version in several locations - excluding those locations which reload version.php mid-flight.

See #37699.

#48 @wonderboymusic
8 years ago

In 38462:

Press This: in get_shortcut_link(), just check a class constant on WP_Press_This instead of instantiating the object and reading an instance prop.

See #37699.

#49 @wonderboymusic
8 years ago

In 38463:

Query: in wp_old_slug_redirect(), use get_query_var() instead of importing and touching the global $wp_query directly.

See #37699.

#50 @wonderboymusic
8 years ago

In 38465:

Press This: in wp_ajax_press_this_save_post() and wp_ajax_press_this_add_category(), don't check for a global instance. WP_Press_This is a Controller, but not really a Singleton. This also keeps it from being a pluggable class, which it is right now.

See #37699.

#51 @wonderboymusic
8 years ago

In 38466:

Press This: don't check for already-hoisted global in press-this.php.

See #37699.

#52 @wonderboymusic
8 years ago

In 38467:

General: use a new function, wp_is_IE(), instead of the $is_IE global in a number of places.

See #37699.

#53 @Compute
8 years ago

It terms of replacing globals I think it would be the time to stop introducing new ones. See for instance #37885

Last edited 8 years ago by SergeyBiryukov (previous) (diff)

#54 in reply to: ↑ 37 ; follow-up: @nacin
8 years ago

Replying to wonderboymusic:

In 38388:

Multisite: move get_current_site() to load.php so that it can be used in more places, instead of importing global $current_site.

See #37699.

This made a multisite-only function accessible in single-site. I don't think that should have been intended, if it was. cc @wonderboymusic, @jeremyfelt.

#55 @johnjamesjacoby
8 years ago

This made a multisite-only function accessible in single-site. I don't think that should have been intended.

FWIW, I see no harm in this.

  • More predictable environment for plugins & themes
  • get_current_blog_id() and get_current_network_id() are already in load.php
  • Hypothetically, these could all live in ms-load.php, but I have a hunch that will require a few additional is_multisite() checks to avoid fatals (would need to try it to see what breaks in single-site)
Last edited 8 years ago by johnjamesjacoby (previous) (diff)

#56 @wonderboymusic
8 years ago

In 38468:

General: revert [38467], wp_is_IE() should not exist.

See #37699.

#57 in reply to: ↑ 54 @jeremyfelt
8 years ago

Replying to nacin:

Replying to wonderboymusic:

In 38388:

Multisite: move get_current_site() to load.php so that it can be used in more places, instead of importing global $current_site.

See #37699.

This made a multisite-only function accessible in single-site. I don't think that should have been intended, if it was. cc @wonderboymusic, @jeremyfelt.

Yes, this change has more impact than reducing the use of a global. I'm not sure that anything really holds us back from doing it (see @johnjamesjacoby's comment), but I'd like rather discuss the decision and possible consequences in a new ticket.

Previously #25158

This ticket was mentioned in Slack in #core by helen. View the logs.


8 years ago

#59 in reply to: ↑ 1 ; follow-up: @MikeSchinkel
8 years ago

Replying to schlessera:

Relying on the WP class name as the Service Locator is not ideal, as a future version of WordPress (PHP 5.3+) would most likely have WP as the root namespace. That future version would probably offer something like WP\Service::get( 'wpdb' );, so something like WP_Service::get( 'wpdb' ); would be preferable. Keeping this as future-proof and flexible as possible should be a priority.

As far as I know there is no conflict here. You can have a WP class and a WP namespace and they won't conflict.

Dependency injection

So, while we cannot yet have a proper injector decide what to inject in what context, we can at least let the constructor take an injected dependency, and provide a "Poka-yoke" for as long as we are not able to do real injection. This would look something like this:

class WP_Comment_Query {

   protected $dbh;

   public function __construct( $query = '', $wpdb = null ) {
      $this->dbh = null !== $wpdb ? $wpdb : WP_service::get( 'wpdb' );
      // [...]
   }
}

Please don't. Let's not start adding lots of positional parameters to constructors so that in the future we have to end up writing code like this:

$query = new WP_Comment_Query(array(...), null, null, null, null, null, null, null, null, new FooBar() );

And please let us not preside over an explosion of classes like WP_service so that to code for WordPress requires us to remember which classes we need to be able to call the methods we need, even though we've mostly be isolated from that since day 1.

There are better ways to handle this to create a much more usable "api", IMO. Everything can route through the WP class using helper classes where the methods are called using __callStatic().

#60 follow-up: @MikeSchinkel
8 years ago

I have real concerns with using WP::get(), e.g. WP::get('wpdb'):

  1. There is no way to type the return value thus coding with an IDE like PhpStorm will result in code that is littered with flagged unresolvable references.
  1. The simple issue that WP::get('wdpb') won't get caught during development in an IDE like PhpStorm because it can't inspect the string and validate it.

Propose instead using WP::<global>() and WP::set_<global>(<new_value>) as in WP::wpdb() and WP::set_wpdb($new_wpdb).

Last edited 8 years ago by MikeSchinkel (previous) (diff)

#61 follow-up: @boonebgorges
8 years ago

[38398] broke BuddyPress's tests. BP had been setting up some initial data in WP page objects in each setUpBeforeClass(), and these objects are now wiped out. It's easily fixed, but worth noting, as there may be others relying on the current behavior. It'd be nice to document the change somewhere other than this ticket, whose relationship to [38398] is not clear. https://buddypress.trac.wordpress.org/ticket/7241

#62 in reply to: ↑ 59 ; follow-up: @schlessera
8 years ago

Replying to MikeSchinkel:

As far as I know there is no conflict here. You can have a WP class and a WP namespace and they won't conflict.

No, there's no technical conflict, but we might want to plan for conventions ahead of time and avoid confusion.

Please don't. Let's not start adding lots of positional parameters to constructors so that in the future we have to end up writing code like this:

$query = new WP_Comment_Query(array(...), null, null, null, null, null, null, null, null, new FooBar() );

With the above example, most people will not need to write any code like this, because of the poka-yoke that makes the most used case the default anyway. But it opens up the possibility to do so, for unit tests, for dependency injection or more advanced scenarios.

Typing characters is cheap. Rewriting an architecture because it misses an extensibility point is not.

[...] so that to code for WordPress requires us to remember which classes we need to be able to call the methods we need [...]

...that's what we do as developers! (well, unless we avoid classes and stuck everything in global namespace, which is "probably a good idea")

There are better ways to handle this to create a much more usable "api", IMO. Everything can route through the WP class using helper classes where the methods are called using __callStatic().

I consider this to be a hack.

#63 in reply to: ↑ 60 ; follow-up: @schlessera
8 years ago

Replying to MikeSchinkel:

I have real concerns with using WP::get(), e.g. WP::get('wpdb'):

  1. There is no way to type the return value thus coding with an IDE like PhpStorm will result in code that is littered with flagged unresolvable references.
  1. This simple issue that WP::get('wdpb') won't get cause during development in an IDE like PhpStorm because it can't inspect the string and validate it.

This is not true, and as every major framework uses containers, PHPStorm has several ways of providing type inspection for them. For a very simple way of adding this, see the article here: https://confluence.jetbrains.com/display/PhpStorm/PhpStorm+Advanced+Metadata

#64 follow-up: @wonderboymusic
8 years ago

Typing happens on the property.

<?php
class Burrito {
    /**
     * @var wpdb
     */
    protected $db;

    public function __construct() {
        $this->db = whatever();
    }
}

Any content store is going to return mixed:

<?php
$foo = wp_cache_get( 'object' );
$foo = wp_cache_get( 'string' );
Last edited 8 years ago by wonderboymusic (previous) (diff)

#65 follow-up: @schlessera
8 years ago

Here's an example of type inspection at work in PHPStorm, through the container indirection:

https://www.alainschlesser.com/wp-content/uploads/2016/09/container_inspection.gif

#66 in reply to: ↑ 62 ; follow-up: @MikeSchinkel
8 years ago

Replying to schlessera:

No, there's no technical conflict, but we might want to plan for conventions ahead of time and avoid confusion.

I see absolutely no potential confusion here. I think you clearly understand the difference, no?

With the above example, most people will not need to write any code like this,

To me "most" is not sufficient. Let's make it all. Inconsistency in programming is a blight on productivity and reliability IMO.

But it opens up the possibility to do so, for unit tests, for dependency injection or more advanced scenarios.

Unless and until ~95% of WordPress developers actively use unit-tests, I'd say this is not a viable solution.

...that's what we do as developers!

I am realizing that I need to write an essay layout out one of the fundamental flaws in developer-think that I have witnessed over my many years; the belief that theoretical perfection is always the ideal and as a fallout of that that the skills required for all programming should be equal, or if not those with lesser skills either need to learn or be forsaken. All too often developers who are skilled think that all developers should always be square pegs whereas there are round holes, triangular holes, etc.

The reality is that we have developers at all skill levels, and we produce the best platform when we recognize and empower all those skill levels to be successful, especially when they align with well-known roles. Here is one way to slice up the different roles and different skill sets in WordPress, all of which can be labeled "developer", in roughly increasing order of skills required:

  1. Custom Theme developer
  2. Custom Plugin developer
  3. Commercial/Open Source Theme Developer
  4. Commercial/Open Source Plugin Developer
  5. WordPress Core contributor
  6. WordPress Core committer

The problem I see is that too many people advocate that we only use techniques that are appropriate for #4-6 and some of #3 but that is way beyond what is needed for #1 & #3 and often #2, and that are often over the heads of the first three roles. Going this approach makes the platform much harder to use for those roles and thus greatly limits its appeal.

In the mid 90s I watched as Microsoft gutted their own Visual Basic user base by releasing VB.NET. Previously Visual Basic had more than 50% marketshare of people who programmed but has since become a footnote in history because Microsoft forsook the market that made Visual Basic popular. (And I think Drupal is currently doing the same to itself.)

Yes, those techniques may be appropriate for WordPress core developers and commercial plugin developers, but the requirement to use them should be limited to that set of developers, not forced on everyone, even the least-skilled custom theme developer who only know copy-and-paste PHP.

Let's put it another way, pushing for everything to require top skill levels ultimately results in far less adoption and shows no empathy for the vast number of people who just program for a living (on non-enterprise level systems) as opposed to programming for art. I even blogged about it in a prior life.

So please, let's stop thinking that every developer has to be Picasso. Frankly, most of them do not want to be great programmers, they just want to get shit done.

(well, unless we avoid classes and stuck everything in global namespace, which is "probably a good idea")

Not sure if you are quoting that for irony or you mean it, but I agree with that. Except I think the global namespace should be a pseudo namespace and not PHP's poorly architected version of namespaces that requires significant develop skill to work with.

I consider this to be a hack.

The are many things in life that are very effective in reality that purists view as a hack. I'd rather have something that is very usable vs. something that is intellectually pure. And I think WordPress as-is is a great example of the former. Show me an intellectually pure that is 1/10th as successful as WordPress?

Last edited 8 years ago by MikeSchinkel (previous) (diff)

#67 in reply to: ↑ 63 ; follow-up: @MikeSchinkel
8 years ago

Replying to schlessera:

This is not true, and as every major framework uses containers, PHPStorm has several ways of providing type inspection for them. For a very simple way of adding this, see the article here: https://confluence.jetbrains.com/display/PhpStorm/PhpStorm+Advanced+Metadata

Yeah, I was afraid that would be offered up as _"the solution."_ As you can see, I have been aware of this capability for a while, and in that link you can see that I think the way PhpStorm implements it is over-the-top complicated and it is very unlikely that most plugin or theme developers will master it.

And yes WP::get() is in core but if included in core it would define the pattern that many developers would mimic in their own plugins and themes and thus create proliferation of code that PhpStorm would flag with errors.

Last edited 8 years ago by MikeSchinkel (previous) (diff)

#68 in reply to: ↑ 64 @MikeSchinkel
8 years ago

Replying to wonderboymusic:

Any content store is going to return mixed:

That is exactly my point. Which is why I am stating that it would not be a good idea to follow that pattern where we do not have to. Clearly for caching we pretty much have to handle any type, but we do not have to for things that are set-in-stone like wpdb.

Minimally I'd argue for both, but I'd prefer that well-known properties just have a "get" and "set" method (where the get method omits the get_ prefix because it is really not needed and just makes code that uses it look more cluttered, especially to non-professional developers such as site builders, custom theme developers and often custom plugin developers.)

#69 in reply to: ↑ 65 @MikeSchinkel
8 years ago

Replying to schlessera:

Here's an example of type inspection at work in PHPStorm, through the container indirection:

Which does not address my other concern:

The simple issue that WP::get('wdpb') won't get caught during development in an IDE like PhpStorm because it can't inspect the string and validate it.

Last edited 8 years ago by MikeSchinkel (previous) (diff)

#70 in reply to: ↑ 67 ; follow-up: @JamesDiGioia
8 years ago

Replying to MikeSchinkel:

Replying to schlessera:

This is not true, and as every major framework uses containers, PHPStorm has several ways of providing type inspection for them. For a very simple way of adding this, see the article here: https://confluence.jetbrains.com/display/PhpStorm/PhpStorm+Advanced+Metadata

Yeah, I was afraid that would be offered up as _"the solution."_ As you can see, I have been aware of this capability for a while, and in that link you can see that I think the way PhpStorm implements it is over-the-top complicated and it is very unlikely that most plugin or theme developers will master it.

And yes WP::get() is in core but if included in core it would define the pattern that many developers would mimic in their own plugins and themes and thus create proliferation of code that PhpStorm would flag with errors.

FWIW, given that PHPStorm has built-in support for WordPress, there's a decent chance they would support the WordPress container OOTB if this landed. I'm not really sure the lack of IDE support should be a blocker here.

Additionally, if you're concerned about hitting as broad a group of developers as possible, IDE concerns aren't that high a priority, given that groups 1-3 probably aren't using PHPStorm anyway.

#71 in reply to: ↑ 70 @MikeSchinkel
8 years ago

Replying to JamesDiGioia:

FWIW, given that PHPStorm has built-in support for WordPress, there's a decent chance they would support the WordPress container OOTB if this landed.

I am just as concerned about the fact that this pattern if included in WordPress will be copied ad infinitum by plugin developers and few it any of these plugins will get the benefit of built-in support in PhpStorm.

I'm not really sure the lack of IDE support should be a blocker here.

Not a blocker, just a request to not create the issue to begin with.

Additionally, if you're concerned about hitting as broad a group of developers as possible, IDE concerns aren't that high a priority, given that groups 1-3 probably aren't using PHPStorm anyway.

There is a saying "Make common cases easy and uncommon cases possible." Using WP::get() this would make those uncommon cases mostly not possible (without a lot of extra work that most people won't know how to do.).

What I am suggesting really is as simple as this:

If we know the name of the global variables we want to refactor -- which we do in all cases -- then let us create named actual methods to access and update them.

BTW, we could use WP::__callStatic() instead of WP::get()` and then document all these global-state methods using PHPDoc; that would be another valid approach. But would be less performant.

#72 in reply to: ↑ 66 ; follow-up: @schlessera
8 years ago

Replying to MikeSchinkel:

You split your argument across two different tickets, so it is difficult to respond. I'll just generally address the issue you're raising here, knowing that this also applies to some of your arguments commented on ticket #36335 .

Your main point is that we fail to keep WordPress development be accessible to "lesser-experienced" developers.

I wholeheartedly agree that a codebase needs to be welcoming and offer a learning curve that is not too steep. However, I think that your conclusions are all backwards.

First of all, an example:

<?php
global $whatever;
$whatever->do_something();

I posit that this is more difficult to comprehend and creates more issues, than something like this:

<?php
WP::get( 'whatever' )->do_something;

The first example creates two separate issues:

  1. You need to explicitly tell the code that you're referencing a global. Apart from immediately showing developers a way of how _not_ to do it, you also assume that they have a basic knowledge of local/global namespace differences.
  2. If the developer uses this in the wrong context (when the global was not initialized yet), it will create very confusing issues/errors that are hard to debug.

The second example improves upon this in the following way:

  1. There's no "pre-qualifying" something before using it. You tell WP what you need, and it will be ready.
  2. If you use this in the wrong context, the WP class can immediately show a clear error explaining why the developer is using this the wrong way. So, the WP class identifies errors during development time, instead of deferring this to the plugin review team.

More generally:

When trying to have a complex system be more approachable for laymen, you build a complex, flexible system first that allows for all advanced usages, and then provide convenience functions/wrappers/facades for the common use cases.

The other way around just doesn't work. You cannot create a more simple system, and the provide more advanced wrappers for advanced use cases. This will result in what we have now with WordPress: you have a more simple system that covers the common use cases, and all the more advanced developers need to use hack and workarounds to solve their problems.

Why do you think that wordpress.com does not use the .org Core? It is simply not usable as is for more complex scenarios...

Your fear probably comes from frameworks like Symfony or Zend. They can account for the most complex sites/apps, but if you want to build a simple blog, you'll need to deal with hugely complex abstractions before getting anything done.

However, I think WordPress should go a similar route like the (opinionated and controversial) Laravel. Laravel has pretty much the exact same abstractions and architectures behind the scenes than Symfony (and even uses most of Symfony Core as is). But it provides its clever Facades for people that don't care about any of this and just want to solve their problem in a few lines of code. More advanced developers, however, can still get direct access to the dependency injection container or create completely custom handlers.

You can always solve a simple problem with a too complex architecture (and preferably wrap a simplification around it). But you will not be able to solve a complex problem with a too simple architecture.

#73 follow-ups: @ChriCo
8 years ago

Howdy.

as i see, @schlessera found enough valid arguments for this "discussion" above, so we can continue the work on a integration of a ServiceContainer into WordPress.

Since we're also working on composer-integration (#36335), we should maybe think - even it's still far far away and no PHP > 5.2 - about the integration of some PSR-"standards". The PSR-group has currently a proposal for the integration of Containers (https://github.com/php-fig/fig-standards/blob/master/proposed/container.md), which should be at least known and respected when developing something similar for WordPress.

Based on the topics above there should also be discussed how the ServiceContainer should be work.

0. Naming

The Container should be called WP_Service_Container and be placed into its own subfolder called service. This - based on my notes above - follows not directly the PSR-standards but in future we maybe switch to namespaces which leads us to PSR-4 (https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-4-autoloader.md) and to WP\Service\Container --> /service/Container.php. Note: i just want to keep - here - the possibility to improve even further in future.. ;)

1. API Design

It needs to be discussed how the API should look like.

WP_Service_Container

This seems to be the major class which has basic implementation of get, set, has. It needs to be discussed what..

  • get returns/throws if something is accessed which is not there.
  • how to implemented protected values which are not overwriteable and how to handle setting a protected value.
  • how to implemented creation of new instances --> e.g. factory - see Pimple http://pimple.sensiolabs.org/#defining-factory-services.
  • what should be extended/implemented to the class (such as ArrayAccess)
  • naming rules of the {{$key}} such as "prefix every internal thing with the "vendor"-name and seperate it with dot's. Configurations are called e.G. wp.config.db) and classes are called e.G. wp.db)
  • LazyLoading of instances when they are first accessed via set
  • ...

Short example of usage:

<?php
$c = new WP_Service_Container();

// ---------------
// set-get-has
$c->set( 'foo', 'bar' );
echo $c->get( 'foo' ); // bar
var_dump( 
        $c->has( 'foo' ), 
        $c->has( 'lorum ipsum' ) 
); // TRUE, FALSE

// ---------------
// protected
$c->protected( 'foo', 'bar' );
$c->set( 'foo', 'b..arrrrr i\'m a pirate' ); // '''not''' possible - exception or at least return FALSE;
echo $c->get( 'foo' ); // bar

// factory
// just an example, since we're not able to use closures we maybe need to build something around it.
// $key, $class_name (instance of class name is the returned value), $args
$c->factory( 'wp.db', 'wpdb', array( 'db-user', 'db-pass', 'db-name', 'db-host' ) );
$wpdb1 = $c->get( 'wp.db' ); // complete new instance of wpdb
$wpdb2 = $c->get( 'wp.db' ); // complete new instance of wpdb

Configurations

The ServiceContainer on its own is a nice thing. But one step further we can think about the generalization of configurations (such as database-credentials, debug modes, ... stuff from wp-config.php and define). The configurations should be "protected" (not changeable) and grouped by "type" such as "db", "debug", ...

Short example to give you a quick view on it:

<?php
// get somehow the service container..
// and set the database configuration as "protected" to disallow overwriting the values.
$service_container->protected( 
        'wp.db.config', 
        array(
                'user' => 'db-user',
                'pass' => 'db-pass',
                'name' => 'db-name',
                'host' => 'localhost'
        )
);

// later on in code when setting up the wpdb
$conf = $service_container->get( 'wp.db.config' );
$service_container->protected( 
        'wp.db', 
        new wpdb(
                $conf[ 'user' ], 
                $conf[ 'pass' ],
                $conf[ 'name' ],
                $conf[ 'host' ]
        )
);

2. $GLOBALS vs. Singleton vs. instance-holding function vs. hook

Since we're hardly depending on some $GLOBALS in every single situation, its maybe not the best choice to add the ServiceContainer to the globals to remove some of them. On the other hand, if we don't rely on a global available class, it's highly possible, that the ServiceContainer will contain static methods and implement some Singleton-Pattern.

v1 - global

Put it to global..than its accessible to everyone and overwritable too..

<?php
$GLOBALS[ 'wp_service_container' ] = new WP_Service_Container();
// ...later on
$GLOBALS[ 'wp_service_container' ]->set( $key, $value );
$value = $GLOBALS[ 'wp_service_container' ]->get( $key );

v2 - singleton

Singleton, no global variable required..but everything is static. Not the best choice here since mocking in UnitTests could be getting much more difficulty.

<?php
WP_Service_Container::get_instance();
// ...later on
WP_Service_Container::set( $key, $value );
$value = WP_Service_Container::get( $key );

v3 - instance holding function

The intance-holding function is a mix of "global"-accessible and singleton, but no static methods/variables required. At least worth to discuss about it.

<?php
function wp_service_container( ) {
        static $instance;
        if ( $instance === NULL ) {
                $instance = new WP_Service_Container();
        }
        
        return $instance;
}

// ...later on
wp_service_container()->set( $key, $value );
$value = wp_service_container()->get( $key );

v4 - hook

The hook seems to be another valid solution here, since we're not having any global variable and its accessible on a very specific point for everyone. The main problem here is, that the complete core has to rely on this single hook to get everything which is stored into the container. So everything which needs access to a $value needs to use this hook, which means we could maybe run into "priority"-problems.

do_action( 'wp_service_container', new WP_Service_Container() );

// ...later on
add_action( 'wp_service_container', function( WP_Service_Container_Interface $container ) {
        $container->set( $key, $value );
        $value = $container->get( $key );
} );

This needs to be discussed since we're still relying heavily on global variables in every single template. I would not implement a Singleton-class and/or something with static methods/variables here, since we're also having a lot of disadvantages with it. A good way could be v3, since we can mock the function in unit tests and access it easily everywhere. But could also be a main disadvantage, if the container is always accessible.


For now i think thats enough input. We should define some basics and afterwards we can continue with more features such es ConfigBuilder's (e.G. WP_Service_ConfigBuilder_PluginConfig whic parses the Plugin file headers:

<?php
$service_container->set( 'my-plugin.config', new WP_Service_ConfigBuilder_PluginConfig( __FILE__ ) );
$config = $service_container->get( 'my-plugin.config' ); 

echo $config[ 'textdomain' ]; // yay, the textdomain!

Have a nice weekend!

Last edited 8 years ago by ChriCo (previous) (diff)

#74 in reply to: ↑ 73 @jacobsantos
8 years ago

I think your design is too "WordPress", in that it condenses what should be multiple methods or classes to a single method. I would also suggest The PHP League Container library. The problem with Pimple or really any container library is that it still needs to be accessed in some way. Something you mention below. Whether that retrieval is in WP or some other Singleton, that class would not be a container. It would hold the container.

I don't believe you looked at MimicCMS Storage implementation. As a POC, I would rather you used it as a foundation since the ideas are:

  1. The storage container should be separate from the retrieval. This should allow for proper testing of each part of the service container without interference of other pieces of WordPress code. Proper unit test cases, instead of the herculean effort found currently in the WordPress test suite to make the test cases as unit as possible. It is easier to unit test something that is meant to be independent than to make something that is not meant to be independent unit testable.
  2. The focus of the service container implementation should be separable from WordPress. This means that you would have another service container implementation in the wild that could gain traction outside of WordPress and therefore support larger than the maintainers. I doubt this would happen as there would likely be a split between what is included in WordPress and the progress of any library. The other, more important, reason is so that the implementation in WordPress could be replaced with another library at a later date. Think WP_HTTP with Requests, except WP_Service with Pimple or League Container.

The reason I don't recommend Pimple is that array access is a bit more difficult to work with than object methods.

<?php
$service = WP::service();
$wpdb = $service['wpdb'];


/// ---------------

$service = WP::service();
$service['wpdb'] = $service->protect(function() {
    return new wpdb;
});
$service['event'] = function() {
    return new WP_Event;
};

Compare that to League's Container library

<?php
WP::service()->share('wpdb', new wpdb());
WP::service()->get('wpdb');

It seems the primary advantage and the reason Pimple is being chosen is because it is simple with few lines of code. Practically, the only thing you would need to do to make it PHP5.2 is to remove the namespace and replace the acceptance of closures. If you want to remove SPL dependency and you should do this anyway, you would just rename the ArrayAccess methods.

I will say that the protection and service layer of MimicCMS Storage is within the classes you pass. It is a better candidate for PHP7 with anonymous classes, but I think it would better serve you to imitate the composition and design of it than Pimple. The PHP League Container library may seem more involved, but once you remove the interfaces and drill down to its code, it isn't any more or less complex than MimicCMS Storage.

What is This?

Replying to ChriCo:

<?php
$c = new WP_Service_Container();
$c->protected( 'foo', 'bar' );
// ---------------
// protected
$c->protected( 'foo', 'bar' );
$c->set( 'foo', 'b..arrrrr i\'m a pirate' ); // '''not''' possible - exception or at least return FALSE;
echo $c->get( 'foo' ); // bar

// factory
// just an example, since we're not able to use closures we maybe need to build something around it.
// $key, $class_name (instance of class name is the returned value), $args
$c->factory( 'wp.db', 'wpdb', array( 'db-user', 'db-pass', 'db-name', 'db-host' ) );
$wpdb1 = $c->get( 'wp.db' ); // complete new instance of wpdb
$wpdb2 = $c->get( 'wp.db' ); // complete new instance of wpdb

Again, the design seems to accept arbitrary data types. This is unwise and I would recommend instead creating a class to contain the data types that are not objects.

Prefer instead:

<?php

final class Storage_Factory_Wpdb implements Storage_FactoryInterface {
    // Implementation creates and stores wpdb instance.
}

final class Storage_Data implements Storage_DataInterface {
    private $data;

    public function __construct($data) {
        $this->data = $data;
    }

    public function get() {
        return $this->data;
    }

    public function set($data) {
        $this->data = $data;
    }
}

final class Storage_ProtectedData implements Storage_DataInterface {
    private $data;

    public function __construct($data) {
        $this->data = $data;
    }

    public function get() {
        return $this->data;
    }

    public function set($data) { }
}

$c = new WP_Service_Container();

$c->protected( 'foo', new Storage_Data('bar') );

// ---------------
// protected
$c->protected( 'foo', new Storage_Data('bar') );
$c->set( 'foo', new Storage_Data("b..arrrrr i'm a pirate") ); // '''not''' possible - exception or at least return FALSE;
echo $c->get( 'foo' )->get(); // bar
 
// factory
// just an example, since we're not able to use closures we maybe need to build something around it.
// $key, $class_name (instance of class name is the returned value), $args
$c->factory( 'wp.db', new Storage_Factory_Wpdb(array( 'db-user', 'db-pass', 'db-name', 'db-host' )) );
$wpdb1 = $c->get( 'wp.db' ); // complete new instance of wpdb
$wpdb2 = $c->get( 'wp.db' ); // complete new instance of wpdb

The concept is more that if you always return an object, then you can do more with it. Take a facade for example.

<?php
class Facade_WP_Database extends WP_Facade {
    protected function _serviceName() {
        return 'wp.db';
    }
}

Facade_WP_Database::insert( 'table', array( 'fieldName' => 'data' ) );
<?php
class Facade_Foo extends WP_Facade {
    protected function _serviceName() {
        return 'foo';
    }
}

echo Facade_Foo::get(); // bar -- from above.

Sure, it borrows much from Laravel, but it is simple enough to implement with a service container. The point is that it doesn't matter what the implementation for registering and retrieving from the service container, because no one is going to use it. Or more that there is a better, easier way to use it.

Tell people that there is an easier API with a Facade and see if people move over to it. Laravel is MIT licensed, you would just have to backport and provide attribution, but given that the implementation is basic and simple, it shouldn't be that difficult to implement, if you don't want to look at Laravel.

That it is more verbose is a nature of supporting a PHP version that doesn't support many of the features in PHP5.3 and above. Since it is just objects and closures are objects, it should be fine to store closures and execute them when retrieved.

It also allows for more composition with the code, which should open the possibilities and extensions a lot more than a simple array would.

#75 @ChriCo
8 years ago

Howdy.

@jacobsantos i'm replying in a short way here:

0. My "implementation
Actually it was no implementation at all. I just throwed in some "requirements" with examples, so everyone here can follow. I think we all agree that we need get, set, has and some way to protect values from overwriting. Also some kind of "creation" (factory) of new instances should be possible. There's also a mix since you should be able to access the current wpdb-connection, but it should also be possible to connect to another db (create a new instance of this class).

In addtion, your StorageFactory (or for Pimple its a ServiceProvider) could improve everything. Since we cannot use closures because of PHP 5.2, we have to build something arround it.

I also had a look at your MimicCMS, but i think thats not the "right" or full solution/way here. Also, as i see it does not support PHP 5.2 so there need to be changed some internals. And thats the point: when we're starting to change it, i would prefer use some well known Container - which is already proved by many many many users (nothing against you and props to your work!) - as basis,

Also we should keep in mind: We've a ton of "APIs" in WordPress and no real packages (e.G. a package for Query (Term, Post, ..), Customizer, AssetManagent (WP_Dependencies..irks) ...). So to add some Factories/Providers are a hugh mess currently.

A good way would be something like:

<?php
.
├─ Query/
|   ├─ Post.php --> WP_Query_Post
|   ├─ Term.php --> WP_Query_Term
|   ├─ ...
|   ├─ composer.json --> haha..just a joke..
|   ├─ Provider/
|   |   ├─  Post.php --> WP_Query_Provider_Post - thats the Factory-implementation of your example above

or something similar. It's just a quick example - naming is not fixed and should just show how it could be implemented decoubled from WordPress with own UnitTests (we've no real UnitTests at all in WordPress...they are more like IntegrationTests..)

1. To Pimple or not
I just mentioned Pimple because it's easy to unterstand, well known, has a very short documention where you can unterstand the complete implementation just by reading some lines. This was just an example, so everyone can get a quick overview what could be implemented. I'm also not liking the ArrayAccess in Pimple, but thats just my personal opinion and that does not mean anything. I'm open to every other well known, tested and documented Container. And there are a lot of Containers and implementations out there: https://github.com/container-interop/container-interop#compatible-projects

2. Seperate it from Core
Since we're stucking with PHP 5.2, we're not able to use some existing implementations. This means, we have to put in a lot of effort to build an own implemtation of it with full test coverage.
I totally agree in this point with you, that the ServiceContainer should be seperated from the Core and loaded via composer. Also it should be decoupled from Core to use it on its own (e.G. no usage of WordPress functions/classes such as {{_doing_it_wrong}} or {{WP_Error}}).

#76 in reply to: ↑ 72 @MikeSchinkel
8 years ago

Replying to schlessera:

Your main point is that we fail to keep WordPress development be accessible to "lesser-experienced" developers.
I wholeheartedly agree that a codebase needs to be welcoming and offer a learning curve that is not too steep.

Finally. I feel prior to now that this point was falling on deaf ears. Thank you for recognizing the concern.

Hopefully others can explicitly state if they agree with this or not. If so, then it can be used as one litmus test for a good approach.

However, I think that your conclusions are all backwards.

Actually, I think you misunderstood my conclusions based on the two examples you contrasted.

I mostly agreed with all you wrote in #comment:72. I agree that WP::get( 'whatever' )->do_something; is better than $GLOBALS['whatever']->do_something();. And while I disagree with your assertion that lesser skilled people will find is easier I think it is not a step too far and one that we should take if we can minimize the learning required to just one more bit is syntax (e.g. ::).

As an aside, I have recent specific experience running monthly WordPress coding workshops for a year. They were ostensibly supposed to be for professional developers but the least skilled people in our group always attended. I really learned a lot about what people who attend WordPress user groups with the intention of learning programming struggle with.

One of key things that trigger them to assume they cannot understand something is when they see syntax they do not understand, especially if they associate it with something they have already decided they can't understand, such as OOP. For example this is a lot of syntax to learn for a non-programmer and it is intimidating to them: WP::get( 'whatever' )->do_something;

To clarify my specific concerns:

FOR THIS TICKET:

  1. The use of a WP::get( 'whatever' ) seems over-architected and with no benefits I can perceive when we can just as easily use WP::whatever(). Especially when whatever is known in advance as it is with all existing globals. Better to create hardcoded methods that each have their own PHPDoc and each can trigger autocomplete in an IDE (without requiring complex machinations to get autocomplete to work.)

GENERAL ISSUES:

  1. For the benefit of archetypes 1 and 2 from #comment:66, let's please avoid adding in namespaces, interfaces and/or dependency injection that themers and site builders would thus need to learn and understand.
  1. And let's try to avoid namespaces and interfaces for archetypes 3-6 whenever it could potentially result in 1 and 2 having to learn them.
  1. When using dependency injection let's (1) use the $args pattern and not a bunch of positional parameters, and (2) always default to the most common case inside the function/method that uses the dependencies. Put a one-time burden on the developer of the function/method to deal with as much of the complexity as possible vs. forcing complexity onto tens of not hundreds of thousands of people who have to deal with it daily.

You can always solve a simple problem with a too complex architecture (and preferably wrap a simplification around it). But you will not be able to solve a complex problem with a too simple architecture.

I strongly agree with that.

But I also believe that we need to go the extra mile to ensure that complexity does not leak out and burden "lesser-experienced" developers. Core (and commercial plugin/theme vendors) should make ever effort to make it trivially easy to use WordPress and their products, respectively, even if it means their own code ends up being very complex.

#77 in reply to: ↑ 73 @MikeSchinkel
8 years ago

Replying to ChriCo:

as i see, @schlessera found enough valid arguments for this "discussion" above, so we can continue the work on a integration of a ServiceContainer into WordPress.

Oh wow. And there I thought your comment was one of the most epic bits of trolling ever seen on Trac -- and I was going to comment "Well Played" -- but instead, you were serious.

#78 follow-up: @F J Kaiser
8 years ago

@MikeSchinkel

The use of a WP::get( 'whatever' ) seems over-architected and with no benefits I can perceive when we can just as easily use WP::whatever(). Especially when whatever is known in advance as it is with all existing globals. Better to create hardcoded methods that each have their own PHPDoc and each can trigger autocomplete in an IDE (without requiring complex machinations to get autocomplete to work.)

Having WP::get( 'dragons' ) is the opposite of over-architected. That is well-architected as it is open for extension – without constantly changing the access layer to the code base as soon as a new object gets implemented. What you are searching for is called convenience-architected. If you believe you need convenience-wrappers, then put a Facade in front of it, as already proposed by @jacobsantos . This is the proper way to hide away complexity.

#79 in reply to: ↑ 78 ; follow-up: @MikeSchinkel
8 years ago

Replying to F J Kaiser:

That is well-architected as it is open for extension – without constantly changing the access layer to the code base as soon as a new object gets implemented.

Is your point that using WP::get( '<global_state>' ) means that anyone could easily add any new global state without having to modify the WP class?

If so, do you believe that making it easy to add global state to an application is a virtue? Especially when you can do so without any place where people would know to go and look for documentation in the source code about your newly added global state?

Last edited 8 years ago by MikeSchinkel (previous) (diff)

#80 in reply to: ↑ 79 ; follow-up: @F J Kaiser
8 years ago

Replying to MikeSchinkel:

Is your point that using WP::get( '<global_state>' ) means that anyone could easily add any new global state without having to modify the WP class?

It seems you missed the point with the previous notes and comments about the global state. Does the first WP_Registry suffer from accessing globals? Yes. But as others have pointed out already, there should be no access to globals, but instances built, stacked and returned – to get rid of the globals part as this is where this ticket has its focus.

If you believe there is a need to discuss the community layer (for e.g. barrier for entry level devs), IDE support or yes-nos for using patterns in general further, I believe that Slack is a better route to address this. Still briefly summed up results of such a discussion would make a good fit in here. This ticket already is hard to follow and comes with a 1h+ reading experience, which raises the entry barrier for devs who would like to participate up to a no-no level.

#81 in reply to: ↑ 80 ; follow-up: @MikeSchinkel
8 years ago

Replying to F J Kaiser:

It seems you missed the point with the previous notes and comments about the global state.

Evidently. Please elaborate on the points I missed.

Does the first WP_Registry suffer from accessing globals? Yes.

I did not call out accessing globals as an issue. Global state does not have to be held in globals to be problematic.

If you believe there is a need to discuss the community layer (for e.g. barrier for entry level devs), IDE support or yes-nos for using patterns in general further, I believe that Slack is a better route to address this.

Since Slack is not segmented by ticket, I have no idea what a valid entry point would be where all those interested in the topic would have any reasonable chance of being involved in the discussion. This ticket is about introducing what I would consider harmful idioms and so it seems the right place to discuss it.

#82 in reply to: ↑ 81 @jacobsantos
8 years ago

@MikeSchinkel I would like to see what your implementation would be. I know you have your library and it is nice. Does wonderboymusic know about it? I would like to see what you mean in a patch?

My interest in this ticket is merely a curiosity, since I do not intend to contribute any code for reasons. My own fork and experiments seem to prove, at least anecdotally and to myself, that the registry pattern combined with facades makes for extremely powerful and simple code. It is much more clean and the concerns are better separated.

I think that Laravel does enough right while attempting to be as simple as possible, that I would rather imitate it at this point. Or at least up until the evolution of code proves a better way. I also don't understand why functions couldn't also be provided for the archetypes or programmers that may not understand OOP or just don't want to deal with objects.

What is wrong with wp_service_wpdb()? or wp_service_wordpress_version()? These functions exist only to return WP::service( 'name' ). Sure, you will have a lot more functions added, but these functions don't really do anything, so they would be short and testing them would be relatively simple. Just check that when the service is registered, they return the object you are looking for or if the service is not registered, they don't.

Functions, if done right, could allow for a functional paradigm composition, which would allow for beautiful construction of building solutions. I am still experimenting with this concept, so I don't have any conclusion. It is more that creating functions that could be composed is difficult. That and using objects with static methods relieves some of the usefulness of using functions by themselves. I believe Laravel also provides functions that forward to static methods. The documentation only describes the functions, so they are more likely to be used in user code


I don't want to discount your experience, even if it conflicts with my beliefs. As much as I would like to believe novice programmers would overcome obstacles given enough time and training. I have known enough people that lacked the intelligence or fortitude to raise above the challenge.

At a certain point, the blame for the student not learning has to be put at the teacher and the methodology employed, until all known methods of teaching and explanation are exhausted. I figure I am just a terrible teacher and part of the reason I don't completely buy-in that novice programmers would be too stupid to put two and two together. Sure some might get some odd number, especially at first while it is still new, but I don't think much of the fundamentals are necessary to eventually conclude 4. Even for the most dense of pupils.

If you say that people have problems with ::, then I will take your word for it, until further evidence proves otherwise. I think with most things, a beginning programmer just has to accept that ClassName::methodName() just works like functionName(), except that there is some weird characters in the signature. For the most part, I would just not explain it. When did a theme author care what they were copying, as long as what they were copying just worked?

Last edited 8 years ago by jacobsantos (previous) (diff)

#83 in reply to: ↑ 16 ; follow-up: @dd32
8 years ago

Replying to wonderboymusic:

In 38275:

Query: add a protected field, $db, (composition, as it were) to WP_*_Query classes to hold the value for the database abstraction, instead of importing the global $wpdb into every method that uses it. Reduces the number of global imports by 32.

See #37699.

Let me start by saying that I personally want to revert this.

This has only changed how the global is accessed, it does nothing to remove it. You can argue that it's reduced the number of locations that we'd need to alter to eventually move to a registry, but it hasn't actually changed the code for any better.
By adding a protected field to every object, when debugging and dumping out lots of objects, it ultimately ends up with a significant proportion of the debug output being references to $wpdb, even though that's not what you're actually inspecting. It gets worse when nested objects each have their own reference to $wpdb in it.

Some people will say that dumping objects is not debugging, and one should use a proper IDE, to that I say yes, but sometimes that's not viable.

So my comment is basically: Does using a private $db field actually benefit reducing globals? given the global is still there, and will remain there, and it's just a reference to the global anyway (So you might as well just call each $this->db reference as a global access).

#84 in reply to: ↑ 83 ; follow-up: @jacobsantos
8 years ago

How is increased data on the debug output a valid argument?

Moving a global to a protected attribute is a valid refactoring technique recommended by experts. Furthermore allows for dependency injection at a later date. Which is considered a good thing.

How is it still a global? If the global changes, the internal object reference will not.

Replying to dd32:

Replying to wonderboymusic:

In 38275:

Query: add a protected field, $db, (composition, as it were) to WP_*_Query classes to hold the value for the database abstraction, instead of importing the global $wpdb into every method that uses it. Reduces the number of global imports by 32.

See #37699.

Let me start by saying that I personally want to revert this.

This has only changed how the global is accessed, it does nothing to remove it. You can argue that it's reduced the number of locations that we'd need to alter to eventually move to a registry, but it hasn't actually changed the code for any better.
By adding a protected field to every object, when debugging and dumping out lots of objects, it ultimately ends up with a significant proportion of the debug output being references to $wpdb, even though that's not what you're actually inspecting. It gets worse when nested objects each have their own reference to $wpdb in it.

Some people will say that dumping objects is not debugging, and one should use a proper IDE, to that I say yes, but sometimes that's not viable.

So my comment is basically: Does using a private $db field actually benefit reducing globals? given the global is still there, and will remain there, and it's just a reference to the global anyway (So you might as well just call each $this->db reference as a global access).

#85 in reply to: ↑ 84 ; follow-ups: @dd32
8 years ago

Replying to jacobsantos:

How is increased data on the debug output a valid argument?

Anything can be a legitimate argument if it affects ones workflows.

Moving a global to a protected attribute is a valid refactoring technique recommended by experts. Furthermore allows for dependency injection at a later date. Which is considered a good thing.

Yes, It's a valid refactoring technique, which adds zero benefit for WordPress. Referencing outside objects via a class method does nothing for code clarity or improving future code. All it does is hide the instance behind yet another layer of abstraction, something that doesn't give us any major benefits. Dependency injection can be done at a later date when it's viable regardless of this change.

How is it still a global? If the global changes, the internal object reference will not.

A reference to a global, verses a direct global import is ultimately the same object, sure, you're accessing it differently, you're creating an extra memory reference to it, but it's still a global object which you're pulling in to every function you use it within.

All this does is give you a fuzzy feeling of "Oh, Look at this, WordPress suddenly has 30 less uses of a global!" when in reality nothing actually changed, and it only added an extra layer in front of the developer for no benefit.

#86 in reply to: ↑ 85 @jacobsantos
8 years ago

Replying to dd32:

Replying to jacobsantos:

How is increased data on the debug output a valid argument?

Anything can be a legitimate argument if it affects ones workflows.

So, a training issue? This is a training issue. It is also a manager's excuse. "Why did you change a perfectly functional UI? Now I have to spend $xx,xxx retraining all of my employee's because you moved a button from location x to location y."

To be honest, I never thought I would hear this from a developer. I guess it makes sense. We developers like to argue over white space and that is one of the most pointless arguments a developer could have. Well, besides text editor. We are fond of our holy wars.

My reasoning is that things change and people either change with it or they don't. Those that don't survive, certainly, but they are stuck. You mentioned using a debugger and I will say that PHP debugger has come a long way. It is unfortunate that there aren't a lot of free choices that make it easy to use xdebug to step through code and set breakpoints. You could look into paying money for a professional license of an IDE that does have good support? There might be a vim plugin or emacs. I don't use emacs, so I wouldn't know.

Actually, the point is that your decision to partially reject progress based on your selfish desire to not have to scroll a little more is really a little disappointing to be honest. I would make a star wars quote, but I have been told it is a terrible movie. Pretty awesome last few minutes, up until the "NOOOOOO!" and "She died of a broken heart" parts. Dat action dough.

Moving a global to a protected attribute is a valid refactoring technique recommended by experts. Furthermore allows for dependency injection at a later date. Which is considered a good thing.

Yes, It's a valid refactoring technique, which adds zero benefit for WordPress. Referencing outside objects via a class method does nothing for code clarity or improving future code. All it does is hide the instance behind yet another layer of abstraction, something that doesn't give us any major benefits. Dependency injection can be done at a later date when it's viable regardless of this change.

Nah, that sounds a lot like micro-optimizations to me. I hear that argument for Python, "Make class attributes local, so that you gain a boost in performance." You could do it, certainly, then after you look at the awful mess you created, you'll go back to doing it the proper way.

I love your argument by the way. The beauty of it is that it can be used for anything in OOP and one of the many, many reasons classes are a mess in WordPress. I do agree that if you don't want to do OOP right, that you shouldn't do it at all, but that ship has already sailed. Classes are terrible in WordPress and it looks like no one knows what they are doing.

This argument, is part of the reason why. I can't believe I have to write this. I know you know already know what I am going to write next, but I'm going to write it anyway. I hope you are trolling. If you are, good show. OOP is nothing, if not abstractions. That is all it is. All you are doing is passing one reference of an abstraction to another concrete example of another abstraction. It is all abstract, until it is not. It is one of those awful paintings that people say is brilliant, but just looks like something a child would finger paint.

I don't know what you mean by an class attribute being an abstraction. I guess in the purely technical sense it is merely the language restricting access to some arbitrary address offset in memory, but from a OOP perspective, it is probably the most concrete thing you can get, other than classes themselves. I would say that attributes would be purely concrete, if you want to go down to the machine layer and think of them as structs. In that sense, you have functions abstracted as methods acting on structs holding data.

Sure going from arbitrary heap location to an arbitrary heap location is probably not that impressive. I would add that the protected variable and the global are both pointing to the same location in memory, until which time something changes. It is harder to reason the copy-on-write rules in PHP as they are slightly different than other languages that actually make sense and actually use pointers. I always forget in PHP5, if you change a member in a class if it holds true for other references to the same object. I could 3v4l that question, but meh that isn't the point.

The point is more that the benefit will come later when you have a nicely abstracted class and everything fits in its place and you don't have to worry about some implementation changing on your, because you don't care about the implementation, you only care that it returns what it should.

How is it still a global? If the global changes, the internal object reference will not.

A reference to a global, verses a direct global import is ultimately the same object, sure, you're accessing it differently, you're creating an extra memory reference to it, but it's still a global object which you're pulling in to every function you use it within.

All this does is give you a fuzzy feeling of "Oh, Look at this, WordPress suddenly has 30 less uses of a global!" when in reality nothing actually changed, and it only added an extra layer in front of the developer for no benefit.

https://github.com/MimicCMS/mimiccms-library/tree/master/src/MimicCMS/WordPress/AdminBar - I'll just put this here. My point is that, I get a warm fuzzy feeling when OOP is done right. Or at least as right as I can attempt to do it.

I don't get warm fuzzy feelings when an application has 30 less uses of a global, because as far as I am concerned with regards to PHP, having 1 global is bad. 30 less of something that could potentially blow up and take the entire application down is 30 less of something I have to worry about. You understand that I could just create a plugin and make $wpdb a global and accidentally create a local variable with the same name and destroy the reference in $wpdb? PHP won't tell you and the condition may not occur but one in a million. Imagine that day when one in a million happens. Imagine that one in a million becomes 1 in 1.

The goal of programming should be to minimize risk. Globals are a huge risk. A protected member is less than a risk. Only a risk for children that inherit from the parent. A global is inherited by the entire application, you change it when it should not be changed and the application is done.

It is less about warm fuzzy feeling than, "Oh dear god! Don't touch the giant red button!" and going around screaming, "You didn't touch the giant red button did you?" Why just not have a giant red button in the first place?


As an aside, I understand that in some languages, globals are a thing and they are fine. Go uses globals and they are great. Part of the reason they are fine is because they are per file. I don't believe you can have a global that is entire application wide, unless you started playing with memory management. Other languages have likewise gotten passed the global issue by not allowing variables to be accessed passed the module barrier. If you access a module global, then you meant to access it.

Version 0, edited 8 years ago by jacobsantos (next)

#87 in reply to: ↑ 85 ; follow-up: @GaryJ
8 years ago

Replying to dd32:

A reference to a global, verses a direct global import is ultimately the same object, sure, you're accessing it differently, you're creating an extra memory reference to it, but it's still a global object which you're pulling in to every function you use it within.

I think there may actually be a change (desired or not) in behaviour here.

With the global $wpdb in each method, you import the latest state of $wpdb when the method is called.

With the global being assigned to the class property in the constructor, you pull in the state at the time the class is instantiated. If the global has been changed between the constructor being called and a method being called, I don't think the method will see that, as it still has $wpdb from the earlier state.

#88 in reply to: ↑ 87 @dd32
8 years ago

Replying to GaryJ:

I think there may actually be a change (desired or not) in behaviour here.

With the global $wpdb in each method, you import the latest state of $wpdb when the method is called.

That was true in PHP4, but from PHP5 onwards all object assignments are done by reference, meaning it's just yet-another-variable which references the global $wpdb contents.

#89 @TimothyBlynJacobs
8 years ago

I think people are talking about two different aspects of object assignment here. If any changes to wpdb state are made to the same global, then those changes will of course be in any property that references wpdb because they are both pointing to the same object.

However, if a plugin stomps the wpdb global, the new object won't be what the class property references.

https://3v4l.org/dBTCg

Last edited 8 years ago by TimothyBlynJacobs (previous) (diff)

#90 @jeremyfelt
8 years ago

In 38636:

Multisite: Revert [38388].

Restore get_current_site() to a multisite only function. Providing this in single site may be a possibility in the future, but should have a dedicated ticket and discussion.

See #37699.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


7 years ago

#92 follow-up: @ramiy
7 years ago

Few more globals that can be replaced with functions:

  • $wp_post_types global can be replaced with get_post_types().
  • $wp_taxonomies global can be replaced with get_taxonomies().
  • $wp_post_statuses global can be replaced with get_post_stati().
  • $wp_roles global can be replaced with wp_roles().
  • $pagenow global can be replaced with get_current_screen().
Last edited 7 years ago by ramiy (previous) (diff)

#93 in reply to: ↑ 92 ; follow-up: @MikeSchinkel
7 years ago

Replying to ramiy:

Few more globals that can be replaced with functions:

Those that return arrays allow access but not modification of the arrays.

#94 in reply to: ↑ 93 @ramiy
7 years ago

Replying to MikeSchinkel:

Replying to ramiy:

Few more globals that can be replaced with functions:

Those that return arrays allow access but not modification of the arrays.

Yes, I know. Using those functions we can't remove all the globals, but we can replace the vast majority.

#95 follow-up: @pento
7 years ago

In 38768:

General: Restore usage of $wpdb, instead of $this->db.

Hiding the $wpdb global behind a property decreases the readability of the code, as well as causing irrelevant output when dumping an object.

Reverts [38275], [38278], [38279], [38280], [38387].
See #37699.

#96 follow-up: @pento
7 years ago

A side note to [38768] - I'm cool with removing globals, but the idea needs to be explored further before it's ready for Core.

#97 follow-up: @schlessera
7 years ago

I totally get that such changes should be properly evaluated, and the Core-to-be-released is probably not the best place to run experiments...

However, I'd like to point out that the goal is not to improve first-time readability (or to even maintain it), but to improve maintainability. Globals like this are technical debt, and I agree that it might be easier to read the code once when you just have them openly accessible like this. It also means that you'll need to read that code in dozens of locations, dozens of times, to get a clearer idea of whether you can make a change or not.

Improving maintainability means that the code might be more complicated to read the first time around. But you won't have to ponder over it for hours to muster up the courage to make a change...

So, yes, I'm all for reverting and discussing how best to tackle this. But we just needn't bother if "first-time readability" is seen as an absolute priority...

#98 in reply to: ↑ 95 @dnaber-de
7 years ago

Replying to pento:

Hiding the $wpdb global behind a property decreases the readability of the code, as well as causing irrelevant output when dumping an object.

Increasing responsibilities of each methods increases readability? How? The advantage of having a dependency (like wpdb) in a property is, that you can rely on its type and existence when you use it. You don't need to care about whether it's in place or even how it came to place.

What you actually do is implementing a shorthand variable. But that could have been done also like that:

<?php
   public function foo() {

       $wpdb = $this->db;
       //...
   }

But instead you import a global variable (which could be of any type, or even not set) to the local scope in every single method. That's actually decreases readability.

#99 in reply to: ↑ 97 ; follow-up: @MikeSchinkel
7 years ago

Replying to ramiy:

Yes, I know. Using those functions we can't remove all the globals, but we can replace the vast majority.

My point was that in order to be able to deprecate the globals we need more than functions that can access them, we'd also need to add functions/functionality than can update them too.


Replying to schlessera:

However, I'd like to point out that the goal is not to improve first-time readability (or to even maintain it), but to improve maintainability.

That presumes there is only one applicable goal and/or that optimizing for maintainability would not have a negative effect on any other goals. Better to verify that there are no other goals the core team thinks are of higher priority than assume that maintainability is the only goal worth optimizing for.

Further, it would be great if you could give some specific use-case examples where the fact that $wpdb is a global variable actually caused real world harm vs. discussing as a theoretical concern.

Personally, while I complete understand and agree with the theoretical concern when discussing an abstract use of global variables I cannot remember one (1) single time that I have run into a problem in my working professionally with WordPress when I had wish for practical reasons that $wpdb was not a global variable. (Okay, there has been one reason and that is for convenience of not having to first declare it global. But that's syntatical sugar, not a maintenance concern, and in our own libraries we have solved that with WPLib::wpdb().)

That said, I would even argue that in WordPress core that most of the well-known globals are not a serious maintainability concern because they are in-fact well-known and there is an army of developers willing and able to maintain any code that contains globals. Sure, it may feel dirty to work with global variables, but what is practical and effective does feel dirty. $wpdb fits into this category.

If for example we deprecating $wpdb and doing so were to harm backward compatibility or even harm understandability among a significant number of the many people who implement WordPress sites but do not understand most of the syntax and semantics of classes and methods then optimizing for maintainability here could be more harmful than not. (I'm not taking a side on this one, I am pointing about that pursuing maintainability should not be viewed without question as the quest for the holy grail.)


OTOH, it would likely make little sense to introduce new globals as WordPress' code base evolves because these potential new globals are not yet well-known.

Last edited 7 years ago by MikeSchinkel (previous) (diff)

#100 @pento
7 years ago

In 38775:

XML-RPC: Re-add a global $wpdb missed in [38768].

See #37699.

#101 @andreasnrb
7 years ago

Bye, bye Miss American Pie
Drove my Chevy to the levee but the levee was dry
And them good ole boys were drinking whiskey and rye
Singin' this'll be the day that globals are reborn
This'll be the day that globals are reborn

:(

#102 in reply to: ↑ 99 ; follow-up: @dnaber-de
7 years ago

Replying to MikeSchinkel:

If for example we deprecating $wpdb and doing so were to harm backward compatibility or even harm understandability among a significant number of the many people who implement WordPress sites but do not understand most of the syntax and semantics of classes and methods then optimizing for maintainability here could be more harmful than not. (I'm not taking a side on this one, I am pointing about that pursuing maintainability should not be viewed without question as the quest for the holy grail.)

FYP. You just say, that people are not willing or able to learn or evolve their skills. With this «argument» you can bury any development of WordPress core. In reverse that means nothing else than teaching people how to write legacy code. This fixes WordPress status as an example of »how to do it wrong«. Congratulations. I'm personally done with core contribution.

#103 in reply to: ↑ 96 @helen
7 years ago

  • Milestone changed from 4.7 to Awaiting Review

Replying to pento:

A side note to [38768] - I'm cool with removing globals, but the idea needs to be explored further before it's ready for Core.

For once, I am forced to agree with @pento. Before this ticket and related discussion get too far into speculation and hand-wringing, I want to be clear that it is absolutely readiness and maintainership that are the real issues at hand. This isn't at a point where its relationship to WordPress's goals or future can be evaluated in any sort of meaningful way. Much like another similarly fated ticket (and to be clear, this should have been reverted at the same time), there is no indication that any proposals were distributed or discussed, concerns from a wide variety of parties addressed (including on this ticket!), or that there would be continuous maintainership of such an effort. I'm sure that the cited concerns in the revert could have been addressed specifically, but that is not an indicator of active ownership - if anything, it's a sign there isn't any.

Contributions from many are highly encouraged, and by no means is every single commenter here expected to stick around and own this, but there needs to be at least one committer AND one other contributing developer (if not a wider working group) who show signs of really owning this kind of a project and process. Neither of those are true here. There are a few of you who have clearly spent significant time reviewing and following this, and there are many more who roll by every once in a while - both are perfectly legitimate and one is not better than the other, but it is not enough to call this a concerted effort.

I don't know what exactly the best process is for this kind of enormous developer-facing effort. Perhaps it's GitHub and treating this as a distinct feature project (which it essentially is), but no matter what, it needs an active owner. Open to thoughts around this as well.

#104 in reply to: ↑ 102 @MikeSchinkel
7 years ago

Replying to dnaber-de:

With this «argument» you can bury any development of WordPress core. In reverse that means nothing else than teaching people how to write legacy code.

You are just setting up a strawman here and justifying with a false binary.

Plus, you missed the point We shouldn't make decisions based on a platitude; there is more than just one valid criteria for changes to core.

Better yet, let us all follow Helen's recommendation and let a core committer lead this project. Once we have one then we can each appeal our concerns to them as they will be in the position of weighing all relevant issues related to WordPress core vs. in a vacuum pursuing an abstract ideal for how code should be written.

#105 @helen
7 years ago

I feel like I should further clarify that anybody can run with a project, you don't have to be a committer by any means. But, it's very hard to feel empowered without a seasoned committer with some deep knowledge of history along for the ride, so I highly recommend starting with one rather than looking for one later.

#106 @lukecavanagh
7 years ago

I would be up for helping but my core history does not go back that far, so my WP core knowledge on previous core trac is more limited to someone who has more experience as well as being a more sesoned committer.

#107 @KalenJohnson
7 years ago

I'm a little confused, @wonderboymusic started this ticket, is a core committer, and has been doing a lot of the work on it. Is he not a core member who is taking the lead on this one?

#108 @alex-ye
7 years ago

Interesting ticket, and nice thoughts, and many long replies.

+1 for the idea, the globals are bad and have too much risk. their usage have to be limited and eventually killed, in the core, plugins and themes.

I think, the core team will have more implementation options/choices when the WordPress requires PHP +5.3, you don't have to rush, if the core team plans to move towards PHP +5.3 soon. for now, why you don't offer some alternatives for the most common globals like $wpdb by maybe a simple function like wp_db(), and encourage plugins/themes authors to use it instead of the global $wpdb;, Sorry if it sounds like a stupid suggestion, but too much global imports in WP code (core/plugins/themes) are there because there is no proper alternative, and until this ticket resolved, many developers still have no choice.

#109 follow-up: @jdgrimes
7 years ago

No reason not to make a long ticket longer, right? So at the risk of sounding preachy (not intended that way, really just thinking out loud here):

I think that a big reason why the core team has difficulty handling tickets like this is that they are too abstract. Most of us, as developers, live in the abstract. Many of WordPress's core committers do a great job at something that many of us sometimes fail at, and that is, living in the real world. To do this, the core team thinks like a user. The users always have problems that they need to solve. WordPress's job is to help them solve those problems.

Now, before you start thinking that that leaves developers out in the cold, remember that to a large degree, WordPress tries to grey the line between the user and the developer. We all understand that this has to be done with care, and that it can have bad side-effects at times. Set that aside for a moment. Because what it really means is that "art" developers like some of us are "users" every bit as much as newbie coders and folks who are too afraid to even copy and paste code. The problem is that the other two groups approach problems differently than we do. They don't have the answers or the solutions. They have the problems. We are the other way around. We have all of the answers (or sometimes think we do ;-), but we can get so immersed into the abstract world of ideal solutions that we forget to do something really simple: have a problem to solve in the first place.

This ticket's OP says that a lot of things are bad. While everybody here, including every last member of the core team, probably agrees on that, it is still subjective and ultimately irrelevant to them (the core team). Because it is irrelevant to the user.

Now, we all know that in one way or another, it really isn't irrelevant to the user. But why? What is the problem? Why is a solution needed? How is the end-user (including the full spectrum of developers), benefited?

The only mention of anything like that in the OP is:

Training people to assume that globals will always be set to the value they expect is ... bad.

Ah, now we have a problem: new developers see us using globals, and that encourages them to use them, too. And we all know that this is a problem because globals can very unexpectedly not have the value that you expect them to have when you least expect it. But people won't know that, because if it ever happened to most of the WordPress globals things would break so bad that we'd notice it right way.

So we could re-word this problem this way: we are encouraging people to write more fragile code, without them even realizing it.

Now that we have a problem, we have a concrete starting point from which to begin diving into our abstract world of solutions. And we can always come back and know for sure whether a proposed "solution" really is one, because we can check whether it solves the problem. (Test-driven development!) The person who comes along and commits that solution will also now know that they are actually accomplishing something, not just churning code "because developers", and exactly why it needs to be done, and that we've thought about how it relates to end-users' (including us!) real-world problems.

Keeping this problem in mind means that the solution isn't necessarily as simple as just removing globals. The goal is to set (and probably provide) a pattern that developers should be following instead of using globals. This means that we need to keep in mind not just reducing fragility, but also making something that developers will actually understand enough to use. Otherwise, they may just revert to something easier, like... I know! Globals FTW!

It also indicates that we may want to actually provide an API that plugin developers can use for their own would-be-globals, not just produce a pattern and then expect them to follow it. Because globals aren't just a pattern, they are a really, really easy to use feature. There is no special implementation in your plugin's code to handle them, no custom classes, functions, etc. You just have the global keyword, and you're all set. So again, in order to actually solve this problem it is better if we provide something as simple as possible.

So now we can look at some concrete constraints to consider as we move forward. And I realize that there are many other constraints, both concrete and abstract, that we could come up with. There are possibly other "problems" that relate to globals that need to be solved. So let's identify 'em and solve 'em. For us! For all the devs!! For the users!! FOR THE WORLD!!! [insert hysterical yell here]


TL;DR: As developers we sometimes jump to solutions too quick, before actually coming up with a clear problem that we are solving so that we (and the core committers) will know when we've found a solution and not just a rabbit trail.

#110 in reply to: ↑ 109 @jacobsantos
7 years ago

I was going to reply long form, but then I realized I agreed with your overall message and didn't want to nitpick the entire message and remove the point. I think part of the problem is that I think we probably all agree that something better should be used, but disagree on what exactly that is.

I agree that focusing on globals at this point is most likely unwise. The problems with globals are not as extreme in a language as PHP. The reason I use such justifications is to support other better patterns. As you have said, the focus should be elsewhere and I think registry or service locator pattern should be included (per also my ticket) as soon as possible. Refactoring existing code to that would be much easier.

I would recommend, given that Composer is also being backported, to backport an existing container library. It should have better support and less errors. When WordPress moves to later PHP versions, you should be able to move to the third-party library with minimal effort, provided the API is compatible.

The reasoning I have used, is that registry or service locator or other inversion of control techniques (plugin, dependency injection, etc) allows for better more focused and isolated unit and functional tests. Being able to properly write integration tests should be the design focus and will help the user in the long run as the system will be more stable and they will experience less bugs as the system will eventually have better unit and functional tests.

I would recommend acceptance tests for system as that might have greater gains in shorter time frame as it will obviously be user focused.

I will not be providing any patches or helping WordPress in any way. It is my policy that it isn't the best use of my time. I will say again that the container libraries has created far better code in my fork and exceeded my expectations with how much better the code looks and functions.

#111 in reply to: ↑ 61 @bobbingwide
7 years ago

Replying to boonebgorges:

[38398] broke BuddyPress's tests. BP had been setting up some initial data in WP page objects in each setUpBeforeClass(), and these objects are now wiped out. It's easily fixed, but worth noting, as there may be others relying on the current behavior. It'd be nice to document the change somewhere other than this ticket, whose relationship to [38398] is not clear. https://buddypress.trac.wordpress.org/ticket/7241

Just to confirm that there were others relying on the current behaviour.
While attempting to run my 'in situ' PHPUnit tests I managed to completely empty two development databases before discovering these changes.
https://github.com/bobbingwide/wordpress-develop-tests/issues/2

Some things are learnt the hard way.

#112 @swissspidy
7 years ago

#32468 was marked as a duplicate.

#113 @xedin.unknown
7 years ago

Hi!

Firstly, I have not read every word of every comment here, so apologies if this has already been mentioned. Recently, I have come across a problem in plugin development, the best solution for which seems to be DI containers. I have found this thread only now, when the solution is mostly ready, so even though I'm not 100% fixed on anything, most of the solution will likely be included in the next version of the plugin I maintain. Allow me to share some of it with you.

Surely, a DI container in WordPress would be great. I understand that it may take a lot of time before we see a new version of WordPress that ships with a DI container implementation, but I hope that this day will come. Which is why I created a function called wp_container() which returns a ContainerInterface. It is only defined if not function exists, so if it is implemented by WordPress or another plugin in the future, it will cause no problems, as long as it does more or less the same thing.

Now, I guess it's worth mentioning that I'm using my own container implementation. This is mostly because I could not find an existing implementation that would be at the same time simple, standards-compliant, and PHP 5.3 compatible (I maintain a line of plugins that require PHP >= 5.3.9). My container accepts definitions in the form of a ServiceProvider via WritableContainerInterface, implements lookup delegation via ParentAwareContainerInterface and CompositeContainerInterface. It is possible to use the same service definitions for creating multiple instances (like if you need to populate an array of objects) by using FactoryInterface.

Back to wp_container(), this function maintains a singleton of ContainerInterface in the form of CompositeContainer via a static variable, and when initializing it for the first time, this object is exposed to the environment via action wp_container_init. Handlers of this action are guaranteed to receive a WritableCompositeContainerInterface, which allows extensions to hook in their own containers with their own service definitions. This allows for a very flexible approach, where all extensions can add their own service definitions using common pattern, but at the same time are in complete control of what happens within their domain.

In my case, I use the wp_container_init hook to attach another composite container to the global one in my main plugin, and then each of my plugin's extensions and the main plugin attach regular Container instances. Like this, I have a single composite container that is responsible for resolving dependencies in my plugin and the plugins' extensions, and then a series of containers, one for each extension, that stores services specific to those extensions. This is very modular and convenient. Because I use lookup delegation and service providers, all my service definitions (i.e. factory closures) accept the main global WordPress container as their first parameter. This means that my services can depend on any other service registered anywhere in the WP environment. Services can, of course, also completely replace other services by being registered with the same name as an existing service. Lookup takes place in LIFO order, so services from containers registered later will override definitions from containers registered before. Services registered by my plugins are prefixed with a plugin code and a . (period), thus forming namespaces to avoid clashes with WP Core or other services.

I hope this helps. Please let me know what you think. If the Core team decides that they want to use my container implementation, I would be willing to work on a stable version, which could slightly change the way I currently do things, for the benefit of all. For example, I plan to replace WritableContainerInterface with a WritableRegistryInterface, so that it could be re-used in other projects that need similar methods, and de-couple it from the ContainerInterface. This would allow developers to not depend on wp_container_init returning a container, but anything that can register services that would later be available via wp_container_init. A separate function in the global namespace will be put in to retrieve FactoryInteface, so as not to require the factory and the container to be the same things (they currently are, but will be separated in a future version, and factory will get DI'd into container). Finally, I have not yet tested with PHP 5.2, but I don't remember dong anything incompatible with PHP 5.2 in the implementation, aside from providing service definitions in the form of closures. This is not required, however, as long as the definition is something that can be invoked, can receive the required params, and can return the service instance. If necessary, I would be willing to include PHP 5.2 in the list of platforms I run tests in, and make some changes to the code if needed. In the future, of course I plan to require a higher version of PHP, but 1.x will be compatible with lower versions.

All the best!

P.S.

I realize how naming my function wp_container() is a bad idea. I've now changed it to something more plugin-specific. But it feels like a good name for a WP function ;)

Last edited 7 years ago by xedin.unknown (previous) (diff)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


7 years ago

#115 @TimothyBlynJacobs
4 years ago

#48963 was marked as a duplicate.

#116 @dingo_d
3 years ago

I didn't want to open a new ticket, but I think this has some impact on the future-proofing WP.

As per RFC https://wiki.php.net/rfc/restrict_globals_usage the $GLOBALS usage will be restricted in PHP 8.1 (the RFC was accepted unanimously).

Scouring the wp core, there is quite a bit of usage of the $GLOBALS variable both in core and tests (https://github.com/WordPress/wordpress-develop/search?q=%24GLOBALS).

I'm wondering if this will have any negative impact on the WP core?

#117 @xedin.unknown
3 years ago

Absolutely, if all services are moved to a DI container, this would play a huge role in future-proofing WP, as well as make it more modular, and compatible.

Note: See TracTickets for help on using tickets.