WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#20465 closed enhancement (fixed)

Change name of global for wp_customize instance

Reported by: mattonomics Owned by: nacin
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.4
Component: Appearance Keywords: has-patch
Focuses: Cc:

Description

Currently, the WP_Customize instance is assigned to $GLOBALS['customize']. I believe this should be prefixed with "wp_". I know this is a bit picky, but it seems to be more in line with the WordPress naming conventions and I don't believe there will need to be any other changes made aside from one line in wp-includes/theme.php

PS Let's see if I can generate a correct patch this time :)

Attachments (1)

theme.php.patch (351 bytes) - added by mattonomics 2 years ago.

Download all attachments as: .zip

Change History (12)

mattonomics2 years ago

comment:1 scribu2 years ago

  • Milestone changed from Awaiting Review to 3.4
  • Summary changed from Change name of global for wp_cusotmize instance to Change name of global for wp_customize instance

+1

comment:2 CoenJacobs2 years ago

+ 1 on this patch. Fully agree.

Shouldn't this be done with all/more $GLOBALS indexes (currently 138 times) in trunk?

comment:3 nacin2 years ago

I've had this queued up locally, just haven't committed it yet.

Wondering if we should move this to being a static instance inside the WP_Customize class and avoid the unnecessary global. So,

new WP_Customize;

And:

public static $instance;
function __construct() {
     self::$instance = $this;
     .  .  .
}

Simple, effective. Allows for WP_Customize::$instance.

comment:4 follow-up: scribu2 years ago

Won't that get weird when you replace WP_Customize with your own instance? (If you can't replace it with your own instance, what's the point of having an instance at all?)

comment:5 in reply to: ↑ 4 ; follow-up: nacin2 years ago

Replying to scribu:

Won't that get weird when you replace WP_Customize with your own instance? (If you can't replace it with your own instance, what's the point of having an instance at all?)

WP_Customize is basically just a namespace. In that situation, $this is convenient, more so over a group of static methods. At the moment, WP_Customize is final. Even if it weren't made final, we could do something to avoid instance overwriting.

Ultimately, getting the instance from WP_Customize just isn't needed in common practice. You'd only need it if you need to unhook something. So it's good to provide a basic interface. But we don't need to go crazy with it. Whatever works, I'm happy with.

comment:6 in reply to: ↑ 5 scribu2 years ago

Replying to nacin:

WP_Customize is basically just a namespace. In that situation, $this is convenient, more so over a group of static methods.

Why is it more convenient? Because you don't have to declare all the properties you set? Because it's a more familiar syntax?

If it's a poor man's namespace, then use it as such. $this is only useful for dynamic dispatch, when you have a hierarchy of classes.

comment:7 nacin2 years ago

You can pass $this to an action in a more dynamic fashion, no? Such as customize_register.

comment:8 scribu2 years ago

Since there can only ever be one instance, you can do it just as well with a static method:

add_action( 'customize_register', array( __CLASS__, 'register_controls' ) );

And to unhook it from a plugin, instead of:

remove_action( 'customize_register', array( WP_Customize::$instance, 'register_controls' ) );

you could do:

remove_action( 'customize_register', array( 'WP_Customize', 'register_controls' ) );
Last edited 2 years ago by scribu (previous) (diff)

comment:9 ciobi2 years ago

  • Cc alex.ciobica@… added

comment:10 ryan2 years ago

  • Component changed from Themes to Customizer

comment:11 nacin2 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [20600]:

Initialize the WP_Customize class in a prefixed global. props mattonomics. fixes #20465.

Note: See TracTickets for help on using tickets.