Page MenuHomePhabricator

Default to Preprocessor_Hash for PHP 7
Closed, ResolvedPublic

Description

HHVM uses Preprocessor_Hash. Currently PHP 7 uses Preprocessor_DOM, but it is slightly incompatible with Preprocessor_Hash (see T216664) and we'd like to standardize on a single preprocessor eventually anyway (T204945).

Regardless of the long-term future of the preprocessor, we should switch to Preprocessor_DOM in our production PHP 7 deployment to avoid incompatibilities.

Event Timeline

cscott created this task.Apr 2 2019, 5:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 2 2019, 5:18 PM
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Krinkle awarded a token.

Do we want to configure it for WMF/PHP7 first (to match what we already do on WMF/HHVM), or do you want to change it in core directly?

cscott added a comment.Apr 9 2019, 5:35 PM

To be conservative, let's configure it for PHP7 first -- since that's an experimental deployment anyway. Then we can talk about changing the defaults once we are certain that we don't come across anything unexpected in production use. (T216664 was unexpected, for instance, at least to me. I'd think the likelihood of being surprised is even lower with a shift to Preprocessor_Hash since we're already using it in production on HHVM... but that's the thing about unknowns: you never know them in advance...)

Changing the default in core could then be done in the same patch as deprecating use of the non-default (T204945), once we are just a little more certain that's what we want to do.

Change 502567 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[operations/mediawiki-config@master] Default to Preprocessor_Hash on both PHP7 and HHVM

https://gerrit.wikimedia.org/r/502567

Change 502567 merged by jenkins-bot:
[operations/mediawiki-config@master] Default to Preprocessor_Hash on both PHP7 and HHVM

https://gerrit.wikimedia.org/r/502567

Joe added a subscriber: Joe.May 7 2019, 4:19 PM

Hi, this is considered a blocker for further deployment of php7. @Krinkle do you think the patch you merged tonight solves the issue and can unblock further deployments?

@cscott my understanding is now that the latest patch is in Parsing needs to look at this again. Are you the right person to pick it up?

This task isn't itself a blocker, it's just about switching the MW core default. I'm un-parenting this in favour of T216664, which is the blocking problem it was meant to solve.

cscott closed this task as Resolved.Jun 14 2019, 4:27 PM
cscott claimed this task.

I'm closing this bug since https://gerrit.wikimedia.org/r/502567 was merged. Technically, we ought to change the shipping defaults to make our production configuration, but we can handle that over in T204945.