WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16918 closed task (blessed) (fixed)

Take out unnecessary compat functions from compat.php

Reported by: markjaquith Owned by:
Milestone: 3.2 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Now that we require PHP 5.2.4, let's take out anything in compat.php that is no longer necessary. If you'd like to tackle this, please say so. Please also verify for certain that the functions you're removing are present in ALL PHP 5.2.4 installs.

Attachments (9)

ticket.16918.diff (5.3 KB) - added by ptahdunbar 3 years ago.
ticket.16918.2.diff (6.7 KB) - added by ptahdunbar 3 years ago.
16918.patch (8.1 KB) - added by hakre 3 years ago.
Some more to go
16918.2.patch (35.2 KB) - added by hakre 3 years ago.
Some more to go
16918.3.patch (35.2 KB) - added by hakre 3 years ago.
_http_build_query() vs http_build_query()
16918.5.patch (36.4 KB) - added by hakre 3 years ago.
_http_build_query() now deprecated
16918.4.patch (35.8 KB) - added by hakre 3 years ago.
build_query() in backwards compatible mode (fix)
16918.diff (431 bytes) - added by aaroncampbell 3 years ago.
Add compat.php back with _()
16918.2.diff (1.2 KB) - added by aaroncampbell 3 years ago.
Add compat.php back with _() & mb_substr()

Download all attachments as: .zip

Change History (39)

ptahdunbar3 years ago

comment:1 ptahdunbar3 years ago

  • Keywords has-patch added; needs-patch removed

Functions in compat.php

  • http_build_query() - PHP 5.0.0
  • _() - PHP 5.0.0
  • stripos() - PHP 5.0.0
  • hash_hmac() - PHP 5.1.2
  • mb_substr() - PHP 5.0.0
  • json_encode() - PHP 5.2.0
  • json_decode() - PHP 5.2.0
  • pathinfo() - updated in PHP 5.2.0

All of them can be removed. I left the file there but empty for future use. In addition, pathinfo52 can be updated to pathinfo where the PATHINFO_FILENAME constant was added used in wp-admin/includes/image-edit.php:554.

comment:2 nacin3 years ago

We should kill the file and the include. We know where to find the template for future functions.

Services_JSON can go too.

ptahdunbar3 years ago

comment:3 ptahdunbar3 years ago

ticket.16918.2.diff removes the compat.php file and replaces instances of _http_build_query() with http_build_query().

comment:4 hakre3 years ago

Related: #16920

hakre3 years ago

Some more to go

comment:5 hakre3 years ago

Consolidated patch, another _http_build_query() was left in class-http.php (the only one I could find from the whole compat functions list), some references to said functions were left in comments.

hakre3 years ago

Some more to go

comment:6 hakre3 years ago

Consolidated patch, removal of class-json.php / Services_JSON as suggested

hakre3 years ago

_http_build_query() vs http_build_query()

comment:7 hakre3 years ago

In #16932, looking into add_query_arg() and friends I've been running over _http_build_query() again.

In this ticket we said, it's compat to http_build_query(), but it's not. The $key parameter is undefined and the $urlencode parameter is undefined.

It looks like that _http_build_query() has been designed pre 5.3.6 (makes sense) and that it's overloading optional parameters that are now used in a different context and therefore render the compat function incompatible.

The udpated patch reflects that for that one call in build_query().

Last edited 3 years ago by hakre (previous) (diff)

comment:8 follow-up: nacin3 years ago

The usage of _http_build_query() is clearly reliant on the requirement to not URL encode there.

comment:9 in reply to: ↑ 8 hakre3 years ago

Replying to nacin:

The usage of _http_build_query() is clearly reliant on the requirement to not URL encode there.

Will provide a fix.

comment:10 hakre3 years ago

Please review.

comment:11 nacin3 years ago

Looks good. We should probably also deprecated _http_build_query() unfortunately.

hakre3 years ago

_http_build_query() now deprecated

hakre3 years ago

build_query() in backwards compatible mode (fix)

comment:12 hakre3 years ago

Next to _http_build_query(), shouldn't we consider wp_clone() to be a "compat" function that's to be deprecated now? There is a patch in Related: #16813.

comment:13 hakre3 years ago

  • Keywords dev-feedback added

Would love to keep the progress going. Some dev-feedback on _http_build_query() and wp_clone is appreciated.

comment:14 ryan3 years ago

(In [17603]) Take out unnecessary compat functions from compat.php. Props hakre, ptahdunbar. see #16918

comment:15 hakre3 years ago

_http_build_query (from php.net manual) is reflected so far (thx ryan, Related: [17603]), wp_clone still pending (is less trivial than previous changes so very much OK).

however would like to see more traction which includes close (esp. in favor of #16920) of this ticket.

Subject to opinion (but?) I think this ticket can already considered closed [that's merely self-criticism].

Feedback appreciated.

Last edited 3 years ago by hakre (previous) (diff)

comment:16 aaroncampbell3 years ago

It looks like in doing this we removed _() which is part of gettext right?If I'm reading this right gettext is optional. Should we be dropping _() back into compat.php and putting it back? See: #17064

aaroncampbell3 years ago

Add compat.php back with _()

comment:17 joostdevalk3 years ago

mb_substr has now been removed, apparently, but mb_ functions are not a default in each PHP install, from http://php.net/manual/en/mbstring.installation.php :

"mbstring is a non-default extension. This means it is not enabled by default. You must explicitly enable the module with the configure option. See the Install section for details."

Please put it back as it was, as it was already throwing errors on yoast.com.

comment:18 aaroncampbell3 years ago

I decided to double check everything:

I don't see why we have _mb_substr() so I combined it with mb_substr() and made a new patch.

aaroncampbell3 years ago

Add compat.php back with _() & mb_substr()

comment:19 markjaquith3 years ago

(In [17620]) Add _() back to compat.php (it is non-default). see #16918. props aaroncampbell

comment:20 markjaquith3 years ago

(In [17621]) Add mb_substr() back to compat.php (it is non-default). see #16918. props joostdevalk

comment:21 markjaquith3 years ago

(In [17622]) Restore compat.php includes. see #16918

comment:22 markjaquith3 years ago

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

I think we're done here.

comment:23 sanb3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Symptom: wordpress 3.2 post-install complains of missing 'hash_hmac' function

Environment: My host is running PHP 5.2.11-pl0-gentoo

Fix: restore hash_hmac to compat.php

comment:24 aaroncampbell3 years ago

What host are you on? If they don't have hash_hmac it means they had to explicitly disable it by using the --disable-hash switch.

comment:25 ryan3 years ago

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

PHP needs to be reemerged with USE=hash.

comment:26 follow-up: lloydbudd3 years ago

Joyent Shared Hosting has PHP Version 5.2.12 (SunOS 5.11), but defaults to --disable-json so is missing json_encode() & json_decode() and that means image editor (backend) oembeds (front end) don't work and (WordPress.com JetPack is also broken).
http://oldwiki.joyent.com/shared:php?s[]=disable&s[]=json

I wonder how common this is. I'm contacting Joyent support to see if they will change their default.

comment:27 bsdguru3 years ago

json.so can be enabled in the relevant php.ini file (~/etc/php5/php.ini or ~/domains/domain.name/etc/php5/php.ini). If it's enabled by default it would not be retroactive - will follow up tomorrow morning about this.

comment:28 in reply to: ↑ 26 ; follow-up: azaozz3 years ago

Replying to lloydbudd:

I'm wondering what is the reason for disabling json. It seems we will be using it more and more, expecting other functions to be transferred to json in 3.3.

comment:29 in reply to: ↑ 28 ; follow-up: lloydbudd3 years ago

Replying to azaozz:

Replying to lloydbudd:

I'm wondering what is the reason for disabling json. It seems we will be using it more and more, expecting other functions to be transferred to json in 3.3.

Joyent's support says it is an optimization to keep the php runtime unbloated.

Looks like it's coming back with #18015: Restore compat Services_JSON (class-json.php), json_encode, json_decode

comment:30 in reply to: ↑ 29 azaozz3 years ago

Replying to lloydbudd:

Yes, seems some hosts have the right version of PHP but no JSON support for no apparent reason. Don't think "unbloated" is good enough excuse, loading it as PHP code would "bloat" it far more :)

Note: See TracTickets for help on using tickets.