WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 14 months ago

Last modified 13 months ago

#27881 closed enhancement (fixed)

Compatibility with Hack

Reported by: wonderboymusic Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: General Keywords:
Focuses: Cc:

Description

We should make sure WordPress is compatible with Hack and can safely (still) run on HHVM.

Attachments (13)

get-terms-27881.diff (1000 bytes) - added by wonderboymusic 15 months ago.
get-posts-27881.diff (1.7 KB) - added by wonderboymusic 15 months ago.
media-27881.diff (11.6 KB) - added by wonderboymusic 15 months ago.
media-27881.2.diff (14.9 KB) - added by wonderboymusic 15 months ago.
media-27881.3.diff (15.3 KB) - added by wonderboymusic 15 months ago.
wp-salt.diff (2.9 KB) - added by wonderboymusic 15 months ago.
plugins_url.diff (1.0 KB) - added by wonderboymusic 15 months ago.
menu-php.diff (1.1 KB) - added by wonderboymusic 15 months ago.
wp_reset_vars.diff (1.2 KB) - added by wonderboymusic 15 months ago.
27881-user.diff (5.7 KB) - added by wonderboymusic 14 months ago.
27881-user.2.diff (5.9 KB) - added by DrewAPicture 14 months ago.
Docs tweaks
27881.opml.diff (1.1 KB) - added by SergeyBiryukov 14 months ago.
27881.diff (6.3 KB) - added by kovshenin 14 months ago.

Download all attachments as: .zip

Change History (105)

comment:1 follow-up: @nacin16 months ago

This is going to be difficult as we use extract() to pull globals into the current (but not global) namespace during template loading. But we could at least get it really close by eliminating unnecessary uses of extract() (there is another ticket for this) and variable-variables, because both are pretty lame.

comment:2 in reply to: ↑ 1 @SergeyBiryukov16 months ago

Replying to nacin:

But we could at least get it really close by eliminating unnecessary uses of extract() (there is another ticket for this)

Related: #22400

comment:3 @wonderboymusic15 months ago

  • Keywords needs-patch added
  • Milestone changed from Future Release to 4.0

First gonna focus on variable-variables

comment:4 @wonderboymusic15 months ago

media-27881.2.diff ditches variable variables and extract() in playlist, audio, and video shortcodes.

comment:5 @wonderboymusic15 months ago

In 28342:

audio, video, and playlist shortcodes:

  • Convert all instances of variable variables to array properties
  • Eradicate use of extract()
  • Rename $atts to $html_atts where collision with new $atts (shortcode atts holder) var might occur

See #22400, #27881.

@wonderboymusic15 months ago

@wonderboymusic15 months ago

comment:6 @wonderboymusic15 months ago

Added patches to ditch variable variables in wp_salt(), plugins_url(), wp-admin/includes/menu.php, wp_reset_vars()

comment:7 @wonderboymusic15 months ago

HHVM ships with a tool called hackificator that analyzes your files and automatically converts them to Hack <?hh or complains. Most of the tools only run on Debian/Linux, so you have to run them in the cloud or a VM using Vagrant.

There are some subtle things Hack complains about. Some are harmless little style fixes. It really hates global scope. I will address the things which don't require a huge refactor first. Some of the refactor items might not be worth the churn, some we won't touch at all.

comment:8 @wonderboymusic15 months ago

In 28477:

The About page has a <div> that doesn't close. Also, <hr>s should self-close. hackificator complains about these things.

See #27881.

comment:9 @wonderboymusic15 months ago

In 28478:

Because the WP_ADMIN constant name can be bound in multiple files, all instances should check ! defined first. wp-admin/admin.php already has this check.

See #27881.

comment:10 follow-up: @wonderboymusic15 months ago

In 28479:

hackificator complains if you call include 'file.php' without the parens, needs to be include( 'file.php' )

See #27881.

comment:11 @wonderboymusic15 months ago

In 28480:

Add a unit test that demonstrates a magic getter.

See #27881.

comment:12 @wonderboymusic15 months ago

In 28481:

Use proper access modifiers and add a magic __get() method to Custom_Background and Custom_Image_Header.

See #27881, #22234.

comment:13 @wonderboymusic15 months ago

In 28482:

hackificator bails on this file because the <meta> isn't self-closing.

See #27881.

comment:14 @wonderboymusic15 months ago

In 28484:

hackificator bails on this file because of mixed quote styles on some HTML attributes.

See #27881.

comment:15 @wonderboymusic15 months ago

In 28485:

In edit-link-form.php, hackificator bails because there is a </form> with no open <form>. It exists, but is needlessly constructed with PHP. It always returns a <form>, only the id and name are different. The dynamic piece just returns the ID now.

See #27881.

comment:16 @wonderboymusic15 months ago

So... been reading more and more.... there are a bunch of things we can't change, but some of these fixes are valid anyways and will at least get us closer.

HHVM can run Hack and PHP, doesn't all have to be Hack-compatible.

A list of things you can't do in Hack: http://docs.hhvm.com/manual/en/hack.unsupported.php

comment:17 @wonderboymusic15 months ago

In 28486:

Add access modifier (public) to members and methods in WP_Comments_List_Table and WP_Post_Comments_List_Table.

See #27881, #22234.

comment:18 @wonderboymusic15 months ago

In 28487:

Add access modifiers to members and methods in WP_Filesystem_Base. Add magic __get() method for backwards compatibility.

See #27881, #22234.

comment:19 @wonderboymusic15 months ago

In 28488:

Add access modifier (public) to methods in WP_Filesystem_Direct.

See #27881, #22234.

comment:20 @wonderboymusic15 months ago

In 28489:

Add access modifier (public) to members and methods in WP_Filesystem_FTPext.

See #27881, #22234.

comment:21 @wonderboymusic15 months ago

In 28490:

Add access modifier (public) to members and methods in WP_Filesystem_ftpsockets.

See #27881, #22234.

comment:22 @wonderboymusic15 months ago

In 28491:

Add access modifier (public) to members and methods in WP_Filesystem_SSH2.

See #27881, #22234.

comment:23 @wonderboymusic15 months ago

In 28492:

Add access modifier (public) to methods in WP_Importer.

See #27881, #22234.

comment:24 @wonderboymusic15 months ago

In 28493:

Add access modifiers to methods and members of list table classes:

  • WP_List_Table is the base class that implements __get() and __call() for BC
  • Adds unit tests to confirm that subclasses properly inherit magic methods
  • Add modifiers to subclasses: WP_Links_List_Table, WP_Media_List_Table, WP_MS_Sites_List_Table, WP_MS_Themes_List_Table, WP_MS_Users_List_Table, WP_Plugin_Install_List_Table, WP_Plugins_List_Table, WP_Posts_List_Table, WP_Terms_List_Table, WP_Theme_Install_List_Table, WP_Themes_List_Table

See #27881, #22234.

comment:25 @wonderboymusic15 months ago

In 28494:

Add access modifiers to methods and members of WP_Users_List_Table.

See #27881, #22234.

comment:26 @wonderboymusic15 months ago

In 28495:

Add access modifier (public) to methods and members of WP_Upgrader_Skin and its subclasses.

See #27881, #22234.

comment:27 @wonderboymusic15 months ago

In 28496:

Add access modifier (public) to methods and members of WP_Upgrader and its subclasses.

See #27881, #22234.

comment:28 @wonderboymusic15 months ago

In 28497:

hackificator doesn't like mixed quote styles in some generated HTML. The switch from single to double allows these files to be parsed.

See #27881.

comment:29 in reply to: ↑ 10 @rmccue15 months ago

Replying to wonderboymusic:

In 28479:

hackificator complains if you call include 'file.php' without the parens, needs to be include( 'file.php' )

See #27881.

We should codify this as a coding standard to ensure we're consistent in the future; we haven't been in the past:

$ ~/bin/ack '^\s*(include|require)(_once)? .*;' | wc -l
     101
$ ~/bin/ack '^\s*(include|require)(_once)\s*\(.*;' | wc -l
     294

comment:30 @wonderboymusic15 months ago

In 28500:

Fix some hackificator odds and ends in wp-admin:

  • wp-activate.php and wp-admin/themes.php don't need the closing PHP tag
  • Switch single quotes for HTML attribute values to double in a few places
  • Convert include_once file.php syntax to include_once( 'file.php' )
  • Add access modifiers to methods/members in: _WP_List_Table_Compat, Walker_Nav_Menu_Edit, Walker_Nav_Menu_Checklist, WP_Screen, Walker_Category_Checklist
  • edit_user() doesn't need to import the $wpdb global
  • wp_list_widgets() doesn't need to import the $sidebars_widgets global
  • switch/endswitch syntax is not supported in Hack
  • A <ul> in wp-admin/users.php is unclosed

See #27881.

comment:31 @wonderboymusic15 months ago

In 28502:

Add access modifiers to methods/members in WP_Object_Cache. Add a magic __get() method for BC.

See #27881.

comment:32 @wonderboymusic15 months ago

In 28503:

Add access modifiers to methods/members in WP_Roles. Add a magic __call() method for BC.

See #27881, #22234.

comment:33 @wonderboymusic15 months ago

In 28504:

Add access modifiers to methods/members in Walker_Category and Walker_CategoryDropdown.

See #27881, #22234.

comment:34 @wonderboymusic15 months ago

In 28505:

Add access modifiers to methods/members in WP_Feed_Cache, WP_SimplePie_File, and WP_Feed_Cache_Transient.

See #27881, #22234.

comment:35 @wonderboymusic15 months ago

In 28506:

Add access modifier to methods of HTTP classes. There are no new private or protected methods, so no need for __call().

See #27881, #22234.

comment:36 @wonderboymusic15 months ago

In 28507:

Add access modifier to methods/members in WP_oEmbed. Adds a magic __call() method for BC.

See #27881, #22234.

comment:37 @wonderboymusic15 months ago

In 28508:

Add access modifier to methods/members in WP_Ajax_Response. Adds a magic __get() method for BC.

See #27881, #22234.

comment:38 @wonderboymusic15 months ago

In 28509:

Add missing access modifiers to methods/members in WP_Customize_*.

See #27881, #22234.

comment:39 @wonderboymusic15 months ago

In 28510:

Add access modifiers to methods/members in WP_Embed.

See #27881, #22234.

comment:40 @wonderboymusic15 months ago

In 28511:

Add access modifiers to methods/members in WP_Error. Add a magic __get() method for BC.

See #27881, #22234.

comment:41 @wonderboymusic15 months ago

In 28512:

Add access modifiers to methods/members in WP_HTTP_IXR_Client.

See #27881, #22234.

comment:42 @wonderboymusic15 months ago

In 28513:

Add missing access modifiers to methods/members in WP_Image_Editor_* classes.

See #27881, #22234.

comment:43 @wonderboymusic15 months ago

In 28514:

Add missing access modifiers to methods/members in Walker and subclasses. Add a magic __get() method.

See #27881, #22234.

comment:44 @wonderboymusic15 months ago

In 28515:

Add missing access modifiers to methods in wp_xmlrpc_server. Add a magic __call() method for BC.

See #27881, #22234.

comment:45 @wonderboymusic15 months ago

In 28516:

Add missing access modifiers to methods in WP and WP_MatchesMapRegex. Add magic __call() and __get() methods to WP_MatchesMapRegex for BC.

See #27881, #22234.

comment:46 @wonderboymusic15 months ago

In 28517:

Add missing access modifiers to methods in WP_Dependencies and _WP_Dependency.

See #27881, #22234.

comment:47 @wonderboymusic15 months ago

In 28518:

Add missing access modifiers to methods in WP_Scripts and WP_Styles.

See #27881, #22234.

comment:48 @wonderboymusic15 months ago

In 28519:

Add missing access modifiers to methods in WP_Comment_Query. Add a magic __call() method for BC.

See #27881, #22234.

comment:49 @wonderboymusic15 months ago

In 28520:

Fix fatal error in unit test.

See #27881.

comment:50 @wonderboymusic15 months ago

In 28521:

Some classes with __get() method also need __set().

See #27881, #22234.

comment:51 @wonderboymusic15 months ago

In 28522:

Add missing access modifiers to methods in WP_Meta_Query.

See #27881, #22234.

comment:52 @wonderboymusic15 months ago

In 28523:

Add missing access modifiers to methods in WP_Query. Add magic methods for __get(), __set(), __isset(), __unset(), and __call().

Add unit test for magic methods.

See #27881, #22234.

comment:53 @wonderboymusic15 months ago

In 28524:

Classes that have __set() also need __isset() and __unset().

See #27881, #22234.

comment:54 @wonderboymusic15 months ago

Here are some issues to look at, might not all be valid, but is the remaining actionable output from hackificator:

wp-admin/includes/class-wp-filesystem-base.php:357:19,26: The method getchmod is undefined in an object of type WP_Filesystem_Base ... wp-admin/includes/class-wp-filesystem-base.php:14:7,24: Check this out
  
wp-admin/includes/class-wp-filesystem-direct.php:24:18,28: You are extending a class that needs to be initialized
 Make sure you call parent::__construct.
 
wp-admin/includes/class-wp-filesystem-ftpsockets.php:22:18,28: You are extending a class that needs to be initialized
 Make sure you call parent::__construct.
 
wp-admin/includes/class-wp-filesystem-ssh2.php:44:18,28: You are extending a class that needs to be initialized
 Make sure you call parent::__construct.
 
wp-admin/includes/class-wp-upgrader-skins.php:28:31,31: Don't use references!
 
wp-admin/includes/class-wp-upgrader-skins.php:702:11,17: The member options is undefined in an object of type Automatic_Upgrader_Skin
   wp-admin/includes/class-wp-upgrader-skins.php:697:7,29: Check this out
   
wp-admin/includes/class-wp-users-list-table.php:388:5,8: Expected modifier 
 
wp-admin/includes/deprecated.php:341:2,4: Expected modifier
 
wp-admin/includes/list-table.php:90:18,38: This is a dangerous method name, if you want to define a constructor, use __construct
 
wp-admin/includes/revision.php:176:35,47: The variable $restore_link is defined
   wp-admin/includes/revision.php:135:4,16: But in a different scope)
   
wp-admin/includes/template.php:38:29,29: Don't use references!
 
wp-admin/options.php:235:12,12: Expected attribute value
 
wp-admin/plugin-editor.php:235:224,224: Expected ul
 
wp-admin/update-core.php:613:20,20: Expected attribute value
 
wp-includes/bookmark-template.php:246:21,25: The variable $cats is defined
  wp-includes/bookmark-template.php:231:3,7: But in a different scope
  
wp-includes/capabilities.php:307:2,4: Expected modifier
  
wp-includes/category.php:344:28,28: Don't use references!
  
wp-includes/class-oembed.php:181:66,71: Undefined variable: $links
  
wp-includes/class-wp-customize-setting.php:392:45,45: Don't use references!
  
wp-includes/class-wp-image-editor.php:277:22,34: The method get_extension is undefined in an object of type WP_Image_Editor
    wp-includes/class-wp-image-editor.php:14:16,30: Check this out
	
wp-includes/class-wp-walker.php:100:29,29: Don't use references!

wp-includes/class-wp-walker.php:333:12,21: The variable $total_top is defined
  wp-includes/class-wp-walker.php:327:4,13: But in a different scope
  
wp-includes/class-wp-xmlrpc-server.php:265:26,26: Don't use references!

wp-includes/class.wp-dependencies.php:122:10,42: Too many arguments

wp-includes/class.wp-scripts.php:172:15,20: The variable $after is defined
  wp-includes/class.wp-scripts.php:159:4,9: But in a different scope	
  
wp-includes/cron.php:327:10,10: Yeah...we're not going to support continue/break N. It makes static analysis tricky and it's not really essential)

wp-includes/date.php:108:2,9: Expected modifier 

wp-includes/default-widgets.php:16:2,9: Expected modifier

wp-includes/class.wp-styles.php:87:10,146: Too many arguments
  wp-includes/plugin.php:156:10,22: Definition is here 
  
wp-includes/query.php:3425:2,9: Expected modifier

wp-includes/revision.php:289:31,31: Don't use references!

wp-includes/rewrite.php:428:2,4: Expected modifier  

wp-includes/script-loader.php:50:30,30: Don't use references!

wp-includes/user.php:452:2,4: Expected modifier

wp/wp-includes/widgets.php:26:2,4: Expected modifier

wp-includes/wp-db.php:64:2,4: Expected modifier

wp-includes/wp-diff.php:34:2,4: Expected modifier

comment:55 @wonderboymusic15 months ago

In 28525:

Add access modifiers to WP_Text_Diff_Renderer_Table that are compatible with its parent class. Some of the inline docs suggest access that, if implemented, would produce fatal errors.

Add magic methods for BC: get(), set(), isset(), unset(), and call().

See #27881, #22234.

comment:56 @wonderboymusic15 months ago

In 28526:

In wpdb, make some things explicitly public. Do not set anything to private. This would instantly blow up hyperdb in the wild.

See #27881, #22234.

comment:57 @wonderboymusic15 months ago

In 28527:

Add public access modifier to methods/members of WP_Widget and WP_Widget_Factory.

See #27881, #22234.

comment:58 @wonderboymusic15 months ago

In 28528:

Add access modifiers to WP_User_Query.

Add magic methods for BC: get(), set(), isset(), unset(), and
call().

See #27881, #22234.

comment:59 @wonderboymusic15 months ago

In 28529:

In WP_Filesystem_Base, the constructor is a noop, so it shouldn't even be declared. Setting it implies that parent::__construct() should be called by its subclasses.

See #27881, #22234.

comment:60 @wonderboymusic15 months ago

In 28530:

hackificator doesn't like mixed single/double-quoted attributes. These were 2 lingering instances in the admin.

See #27881.

comment:61 @wonderboymusic15 months ago

In 28531:

Upgrade _WP_List_Table_Compat to PHP5-style constructor.

Add public to methods/members of WP_Role.
Add public to methods/members of WP_User where appropriate. Don't set private where indicated until more study has occurred and tests have been written for compatibiliy with existing magic methods.

See #27881, #22234.

comment:62 @wonderboymusic15 months ago

In 28532:

WP_Date_Query was only missing one access modifier.

Add access modifier (public) to all default widgets' class methods.

See #27881, #22234.

comment:63 @wonderboymusic15 months ago

In 28533:

WP_Query was only missing one access modifier.

Add access modifier (public) to applicable class methods/members of WP_Rewrite. I am not brave enough to set some of the vars to private without more testing.

See #27881, #22234.

comment:64 @wonderboymusic14 months ago

In 28633:

Cleanup for switch statements:

  • Move default to the bottom in WP_Theme_Install_List_Table
  • switch/endswitch syntax is not supported in Hack. Switch to switch (...) { .... } syntax. (A few template-type instances linger).

Fixes #28409.
See #27881.

comment:65 @wonderboymusic14 months ago

In 28734:

Don't use a variable variable in wp_widget_rss_form(). Sidenote: the logic to show hidden fields is bizarre - would result in duplicate fields.

See #27881.

comment:66 @ircbot14 months ago

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.

comment:67 @wonderboymusic14 months ago

In 28736:

Don't use variable variables in get_terms().

See #27881.

comment:68 @wonderboymusic14 months ago

In 28737:

Don't use variable variables in WP_Query::get_posts().

See #27881.

comment:69 @wonderboymusic14 months ago

In 28738:

Don't use variable variables in WP_Comment_Query::query().

See #27881.

@DrewAPicture14 months ago

Docs tweaks

comment:70 @wonderboymusic14 months ago

In 28739:

Don't use variable variables in plugins_url().

See #27881.

comment:71 @wonderboymusic14 months ago

In 28740:

Don't use variable variables in wp_insert_user().
Add a local array, $meta, to provide substantial disambiguation among variables.

See #27881.

comment:72 @wonderboymusic14 months ago

In 28741:

Don't use variable variables in wp_salt().

See #27881.

comment:73 @wonderboymusic14 months ago

In 28742:

Don't use variable variables in wp_dashboard_plugins_output(). Variable variables aren't the worst thing about this function.

See #27881.

comment:74 @wonderboymusic14 months ago

In 28743:

OPML:

  • Learn how OPML works, cry.
  • Remove unused globals in startElement()
  • Stop using variable variables in startElement()

Tested with the OPML importer plugin

See #27881.

comment:75 @wonderboymusic14 months ago

In 28744:

Don't use variable variables in setup-config.php (Step 2).
See #27881.

comment:76 @wonderboymusic14 months ago

In 28745:

Don't use variable variables in user-new.php. Test by causing errors when creating a new user.

See #27881.

comment:77 @wonderboymusic14 months ago

In 28746:

Don't use variable variables in wp_reset_vars(). Test by searching in list tables, etc.

See #27881.

comment:78 @wonderboymusic14 months ago

In 28747:

Don't use variable variables in touch_time().
See #27881.

comment:79 @wonderboymusic14 months ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from new to closed

Got rid of extract() and variable variables ($$name). Added access modifiers to classes. Fixed some PHP class definitions. Eradicated a lot of mixed quote styles in HTML.

We are nowhere near close to being 100% compatible with Hack. Hack doesn't support global scope!

Cleaned up a lot. Done for now.

comment:80 @SergeyBiryukov14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

27881.opml.diff (untested) removes obsolete globals from docs. $opml_map and $map seem to be unused after [28743].

@kovshenin14 months ago

comment:81 @kovshenin14 months ago

Looks like the logic in r28734 has changed. $$input used sanitized variables which contained actual values, unlike $inputs[$input] which in that context contains data about which input fields are hidden. See 27881.diff.

comment:82 @kovshenin14 months ago

In r28736 the isset() logic seems to be redundant, it will return true for everything because all the variables are defined at one point or another.

comment:83 @wonderboymusic14 months ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from reopened to closed

In 28786:

Remove unused globals from link-parse-opml.php after [28743].

Props SergeyBiryukov.
Fixes #27881.

comment:84 @wonderboymusic14 months ago

In 28787:

Cleanup wp_widget_rss_form() after [28734]. "$$input used sanitized variables which contained actual values, unlike $inputs[$input] which in that context contains data about which input fields are hidden."

Props kovshenin.
Fixes #27881.

comment:85 @DrewAPicture13 months ago

In 29139:

Fill out inline documentation for magic methods added to the WP_Text_Diff_Renderer_Table class in [28525].

See #27881, #22234 and #28885.

comment:86 @DrewAPicture13 months ago

In 29140:

Fill out inline documentation for magic methods added to the WP_User_Query class in [28528].

See #27881, #22234 and #28885.

comment:87 @DrewAPicture13 months ago

In 29141:

Fill out inline documentation for magic methods added to the WP_Query class in [28523].

See #27881, #22234 and #28885.

comment:88 @DrewAPicture13 months ago

In 29142:

Fill out inline documentation for magic methods added to the WP_MatchesMapRegex class in [28516].

See #27881, #22234 and #28885.

comment:89 @DrewAPicture13 months ago

In 29143:

Fill out inline documentation for magic methods added to the Walker class in [28514].

See #27881, #22234 and #28885.

comment:90 @DrewAPicture13 months ago

In 29144:

Fill out inline documentation for magic methods added to the WP_Error class in [28511].

See #27881, #22234 and #28885.

comment:91 @DrewAPicture13 months ago

In 29145:

Fill out inline documentation for magic methods added to the WP_Ajax_Response class in [28524].

See #27881, #22234 and #28885.

comment:92 @DrewAPicture13 months ago

In 29146:

Fill out inline documentation for magic methods added to the WP_Object_Cache class in [28502], [28521], [28524].

See #27881, #22234 and #28885.

Note: See TracTickets for help on using tickets.