Page MenuHomePhabricator

tstarling (Tim Starling)
UserAdministrator

Projects (19)

Today

  • Clear sailing ahead.

Tomorrow

  • Clear sailing ahead.

Thursday

  • Clear sailing ahead.

User Details

User Since
Oct 15 2014, 8:27 PM (257 w, 5 d)
Roles
Administrator
Availability
Available
LDAP User
Tim Starling
MediaWiki User
Tim Starling (WMF) [ Global Accounts ]

Recent Activity

Yesterday

tstarling added a comment to T231580: Implement GET Revision Comparison.

I closed T232488 with a comment summarising the discussion between me and @eprodromou regarding /json suffixes. Everything is JSON. I'm not sure if we resolved on the exact name of the /structured suffix but I think it would be confusing to use /json when all the formats are a kind of JSON, hence the need to think of another name for it.

Mon, Sep 23, 11:53 PM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling closed T232488: "json" type indicator in URL patterns, a subtask of T231338: iOS client history API, as Declined.
Mon, Sep 23, 11:40 PM · CPT Initiatives (Core REST API in PHP), Epic, Core Platform Team Workboards (Epics)
tstarling closed T232488: "json" type indicator in URL patterns as Declined.

This is not really what I was attempting to ask for, as I just discussed with @eprodromou. Everything is JSON, so /json is redundant. My concern is just about changing what's inside the JSON depending on the needs of the client, for example the three diff formats I described at T231580. I don't think there is a simple top-level solution for that, we just have to think about future use cases on some of these endpoints.

Mon, Sep 23, 11:40 PM · Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling updated the task description for T231580: Implement GET Revision Comparison.
Mon, Sep 23, 5:47 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231580: Implement GET Revision Comparison.

In the WIP patch, instead of "from_revision" and "from_title" I had a nested structure:

{
  "response": {
    "from": {
      "id": 1522,
      "title": "Main_Page"
    },
    "to": {
      "id": 1526,
      "title": "Main_Page"
    },
Mon, Sep 23, 5:45 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231580: Implement GET Revision Comparison.

Is the title meant to be the DB key, or the text form, or the display title (HTML formatted), or a plain text representation of the display title? If it's the display title then should it be in the user's variant?

Mon, Sep 23, 4:37 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling updated subscribers of T231580: Implement GET Revision Comparison.

It's not possible to do an IMS/INM revalidation of a 403 or 404 response, since a 304 status code is defined as implying that the status code would have been 200. So the best we can do for those responses is a short Expires.

Mon, Sep 23, 3:55 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231580: Implement GET Revision Comparison.

Some additional considerations which apply to either interface style:

Mon, Sep 23, 2:51 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231580: Implement GET Revision Comparison.

In the content negotiation version you would also have a directory index style endpoint /revision/{id}/compare/{other}. This would just list the role name of each modified slot.

Mon, Sep 23, 2:14 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231580: Implement GET Revision Comparison.

Content negotiation version of this, for consideration:

Mon, Sep 23, 2:09 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231580: Implement GET Revision Comparison.

I made the following changes. Please revert if any are unacceptable:

  • Added a format parameter to the path, here /structured, so that we may also add /table and /inline.
  • Changed method specification from GET only to GET or HEAD, to conform with the HTTP specification and existing routing code (T226043)
  • Removed the requirement that the request body be empty. RFC 2616 prohibits such a request body, whereas RFC 7231 says that such a request body has "no defined semantics", which might be interpreted as a suggestion to ignore the body, which is the normal handling of such requests. In any case, the endpoint seems like the wrong place to implement such a requirement. I don't think any endpoint should interpret a GET request body: RFC 7231 says that "sending a payload body on a GET request might cause some existing implementations to reject the request", which seems like a good enough reason to not do that. Proxies may fail to forward it. The best way to enforce this handling would be to have RequestFromGlobals::getBody() always return an empty stream for a GET request.
  • Specified that "diffs" contains a JSON object mapping MCR slot role name to the result of wikidiff2_inline_json_diff(), rather than "TBD".
  • Renamed "title" to "from_title" and added "to_title" since there was no requirement that the revision IDs refer to the same page.
  • For clarity given the above change, renamed "from" and "to" to "from_revision" and "to_revision" respectively.
  • It said that a 304 should be generated if there is "no change between revisions". I'm not sure if this is just unclear wording, but RFC 7232 defines the 304 status code as something which can only be used in response to IMS/INM, it is not possible to send it if the from and to revisions merely contain the same content. If the from and to revisions merely contain the same content, the diff array will be empty, thus allowing the client to receive the title and other metadata.
Mon, Sep 23, 1:53 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling updated the task description for T231580: Implement GET Revision Comparison.
Mon, Sep 23, 1:53 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231580: Implement GET Revision Comparison.

Since there was no response to my questions here, I'll just update the task description to address these problems.

Mon, Sep 23, 12:45 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)

Sat, Sep 21

Volker_E awarded T232563: Drop IE6 and IE7 basic compatibility and security support a Like token.
Sat, Sep 21, 2:05 AM · MediaWiki-General, TechCom-RFC

Fri, Sep 20

MusikAnimal awarded T232563: Drop IE6 and IE7 basic compatibility and security support a Like token.
Fri, Sep 20, 7:11 PM · MediaWiki-General, TechCom-RFC

Tue, Sep 17

tstarling renamed T232485: RFC: Core REST API namespace and version from Namespace and version to Core REST API namespace and version.
Tue, Sep 17, 11:06 PM · TechCom-RFC, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a project to T232485: RFC: Core REST API namespace and version: TechCom-RFC.

I think this task should be an RFC. It proposes a set of public endpoints which will inevitably be used by clients other than iOS. It proposes a path scheme which implies a trivial extension (v0 -> v1) to a permanent, official, public API. It proposes introducing a /core namespace despite contrary views on that point.

Tue, Sep 17, 10:59 PM · TechCom-RFC, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)

Mon, Sep 16

tstarling added a comment to T232613: LBFactoryMulti.php: PHP Notice: Undefined index: .

The theory that zend_empty_string fails to have its hash value initialised when opcache is enabled was not correct. It is already initialised before php-fpm forks. So the new theory is that it is initialised correctly but something sets it back to zero.

Mon, Sep 16, 7:09 AM · Patch-For-Review, MW-1.34-notes (1.34.0-wmf.22; 2019-09-10), Core Platform Team Workboards (Clinic Duty Team), Wikimedia-Rdbms, PHP 7.2 support, Wikimedia-production-error

Sat, Sep 14

Daimona awarded T232563: Drop IE6 and IE7 basic compatibility and security support a Like token.
Sat, Sep 14, 1:03 PM · MediaWiki-General, TechCom-RFC
tstarling added a comment to T232613: LBFactoryMulti.php: PHP Notice: Undefined index: .

Changing ILoadBalancer::GROUP_GENERIC to a string of length greater than 1 might also work as a permanent workaround. It could be say '[generic]'.

Sat, Sep 14, 10:19 AM · Patch-For-Review, MW-1.34-notes (1.34.0-wmf.22; 2019-09-10), Core Platform Team Workboards (Clinic Duty Team), Wikimedia-Rdbms, PHP 7.2 support, Wikimedia-production-error
tstarling added a comment to T232613: LBFactoryMulti.php: PHP Notice: Undefined index: .

This should be tried as a workaround: opcache.interned_strings_buffer = 0 in php.ini.

Sat, Sep 14, 8:45 AM · Patch-For-Review, MW-1.34-notes (1.34.0-wmf.22; 2019-09-10), Core Platform Team Workboards (Clinic Duty Team), Wikimedia-Rdbms, PHP 7.2 support, Wikimedia-production-error
tstarling added a comment to T232613: LBFactoryMulti.php: PHP Notice: Undefined index: .

Continuing the core dump analysis:

Sat, Sep 14, 5:05 AM · Patch-For-Review, MW-1.34-notes (1.34.0-wmf.22; 2019-09-10), Core Platform Team Workboards (Clinic Duty Team), Wikimedia-Rdbms, PHP 7.2 support, Wikimedia-production-error

Fri, Sep 13

tstarling added a comment to T232613: LBFactoryMulti.php: PHP Notice: Undefined index: .

I'll dump my notes here, sorry about it being long and boring. Summary: no answers yet.

Fri, Sep 13, 12:38 PM · Patch-For-Review, MW-1.34-notes (1.34.0-wmf.22; 2019-09-10), Core Platform Team Workboards (Clinic Duty Team), Wikimedia-Rdbms, PHP 7.2 support, Wikimedia-production-error
tstarling added a comment to T232613: LBFactoryMulti.php: PHP Notice: Undefined index: .

It didn't generate a core dump. In my defence I did only say that it would "probably be enough". It doesn't work because PR_SET_DUMPABLE is used to disable core dumps. To enable core dumps, the PHP FPM pool option process.dumpable needs to be set to "yes". This can safely be set to "yes" globally since rlimit_core still defaults to zero, so it doesn't dump core on segfault unless the limit has been manually raised. You can see the current configuration with php-fpm7.2 -tt, and I'm also going to upload a test script.

Fri, Sep 13, 4:46 AM · Patch-For-Review, MW-1.34-notes (1.34.0-wmf.22; 2019-09-10), Core Platform Team Workboards (Clinic Duty Team), Wikimedia-Rdbms, PHP 7.2 support, Wikimedia-production-error

Thu, Sep 12

tstarling added a comment to T232485: RFC: Core REST API namespace and version.

I understand Daniel's desire to avoid putting a collection of half-baked endpoints into production with generic unversioned names. How about we split the whole iPhone app support project into an extension and give it its own prefix? Then it can act as a prototype for similar endpoints to be introduced to core.

Thu, Sep 12, 11:32 PM · TechCom-RFC, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling moved T231580: Implement GET Revision Comparison from Ready to Doing on the Core Platform Team Workboards (Green) board.
Thu, Sep 12, 5:57 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231580: Implement GET Revision Comparison.

Why is it diffs plural? A diff is what you get when you compare two revisions.

Thu, Sep 12, 5:54 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231347: Curator compares revisions.

Can the request/response details be removed from the task description, to avoid duplicating the discussion? It's all at T231580.

Thu, Sep 12, 5:51 AM · CPT Initiatives (Core REST API in PHP), Core Platform Team Workboards (User Stories), Story
tstarling added a comment to T231580: Implement GET Revision Comparison.

To repeat my comment from T231347: The diff format is needed, and JSON is not one of the options. I think the options are "table" (wikidiff2_do_diff), "inline" (wikidiff2_inline_diff) and "structured" (wikidiff2_inline_json_diff, T232231). So we would have e.g. /revision/{id}/compare/{other}/table , which would have HTML in JSON. For "structured", hopefully it is not necessary to double-encode the JSON.

Thu, Sep 12, 4:45 AM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231347: Curator compares revisions.

The diff format is needed, and JSON is not one of the options. I think the options are "table" (wikidiff2_do_diff), "inline" (wikidiff2_inline_diff) and "structured" (wikidiff2_inline_json_diff, T232231). So we would have e.g. /revision/{id}/compare/{other}/table , which would have HTML in JSON. For "structured", hopefully it is not necessary to double-encode the JSON.

Thu, Sep 12, 4:42 AM · CPT Initiatives (Core REST API in PHP), Core Platform Team Workboards (User Stories), Story
tstarling added a comment to T232485: RFC: Core REST API namespace and version.

I would support having a major version number which is associated with each route, not a global version number. It should start at v1, not v0. I think it should be incremented extremely rarely: in most of the cases which @Anomie mentions above, it would not be incremented. I don't think we should commit to supporting old versions forever. For example, if you need to add a CSRF token to a vulnerable endpoint, you could add the v2 endpoint and remove the v1 endpoint in the same commit.

Thu, Sep 12, 4:10 AM · TechCom-RFC, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T231347: Curator compares revisions.

I renamed version to revision, as discussed.

Thu, Sep 12, 3:08 AM · CPT Initiatives (Core REST API in PHP), Core Platform Team Workboards (User Stories), Story
tstarling renamed T231347: Curator compares revisions from Curator compares versions to Curator compares revisions.
Thu, Sep 12, 3:07 AM · CPT Initiatives (Core REST API in PHP), Core Platform Team Workboards (User Stories), Story
tstarling closed T230914: User authentication with OAuth 1.0, a subtask of T229662: Minimal client REST API, as Resolved.
Thu, Sep 12, 3:03 AM · Core Platform Team Workboards (Epics), Epic, CPT Initiatives (Core REST API in PHP)
tstarling closed T230914: User authentication with OAuth 1.0 as Resolved.

As far as I can see, there are no proposed write actions, so CSRF should not be a problem initially. Allowing OAuth but ignoring session authentication is not "keeping it simple", because nothing in MediaWiki does that, whereas allowing both forms of authentication was done with a few lines of code and has now been merged.

Thu, Sep 12, 3:03 AM · Core Platform Team Workboards (Green), Story, CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T229661: Core REST API in MediaWiki.

If it is meant to be the umbrella for work planned for CPT for some time period, I think a project is the right way to indicate that also. But the project name should specify the scope.

Thu, Sep 12, 2:59 AM · Core Platform Team, CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T229661: Core REST API in MediaWiki.

This appears to be a tracking task, not a completable project. Per https://www.mediawiki.org/wiki/Phabricator/Project_management/Tracking_tasks , tracking tasks should not be used. I think these tasks should be put in the MediaWiki-REST-API project.

Thu, Sep 12, 2:49 AM · Core Platform Team, CPT Initiatives (Core REST API in PHP)

Wed, Sep 11

tstarling added a comment to T232563: Drop IE6 and IE7 basic compatibility and security support.

The same kind of query also confirms that IE7 users are mostly seeing redirects, whereas IE8 still works. Of course it would be possible to extend support for non-WMF users of MediaWiki, but I don't think we should do that.

Wed, Sep 11, 9:11 PM · MediaWiki-General, TechCom-RFC
tstarling added a comment to T232563: Drop IE6 and IE7 basic compatibility and security support.

@Krinkle pointed out that IE6 and IE7 attempting to visit Wikipedia are typically redirected to an error page due to not supporting current TLS ciphers. I confirmed that this is the case using the webrequest_sampled_128 table in analytics via Turnilo. For UAs containing "MSIE 6.0", 71% of requests result in a 301 status code, whereas for all UAs, 3% of requests result in a 301 status code. So IE6 and IE7 are already thoroughly broken for readers.

Wed, Sep 11, 9:01 PM · MediaWiki-General, TechCom-RFC
Jdforrester-WMF awarded T232563: Drop IE6 and IE7 basic compatibility and security support a Like token.
Wed, Sep 11, 3:50 PM · MediaWiki-General, TechCom-RFC
tstarling added a project to T232563: Drop IE6 and IE7 basic compatibility and security support: MediaWiki-General.
Wed, Sep 11, 5:27 AM · MediaWiki-General, TechCom-RFC
tstarling created T232563: Drop IE6 and IE7 basic compatibility and security support.
Wed, Sep 11, 5:10 AM · MediaWiki-General, TechCom-RFC
tstarling added a comment to T198492: Create a maintenance script to drop rev_text_id and ar_text_id from the database..

Maybe that was true 15 years ago, but update.php seems well-established now.

Wed, Sep 11, 1:56 AM · MW-1.35-release, CPT Initiatives (MCR), Multi-Content-Revisions (Tech Debt), Wikidata

Tue, Sep 10

tstarling added a comment to T201965: Vagrant setup produces exception about $wgServer when https role is active.

I think protocol-relative URLs should be deprecated. If you really need a wiki to be accessible via both HTTP and HTTPS, you can always have a dynamic $wgServer, but all the hacks that help to switch between HTTP and HTTPS and the support for cache sharing between the two should just be removed. HTTPS should be used for public websites, and HTTP for localhost. There's not really a situation in which switching between the two is advisable.

Tue, Sep 10, 11:33 PM · MediaWiki-Vagrant
tstarling claimed T231580: Implement GET Revision Comparison.
Tue, Sep 10, 12:09 PM · Patch-For-Review, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling moved T230914: User authentication with OAuth 1.0 from Waiting for Review to Done on the Core Platform Team Workboards (Green) board.
Tue, Sep 10, 12:05 PM · Core Platform Team Workboards (Green), Story, CPT Initiatives (Core REST API in PHP)

Sun, Sep 8

tstarling added a comment to T230492: Requesting SRE permissions to create Gerrit projects under operations/debs.

@herron do you want to be in the Gerrit Manager group? That will allow you to create projects.

Sun, Sep 8, 11:45 PM · Gerrit-Privilege-Requests

Sat, Sep 7

tstarling added a comment to T232224: September 2019 DoS attacks [Public].

Feel free to delete the comment if my trouble has nothing to do with the incident, but a hour ago Ī̲ found myself unable to hear any TCP reply from text-lb.eqiad.wikimedia.org[208.80.154.224] querying it from exactly one IP.

Sat, Sep 7, 10:31 AM · Wikimedia-Incident, Operations
tstarling lowered the priority of T232224: September 2019 DoS attacks [Public] from Unbreak Now! to High.
Sat, Sep 7, 5:29 AM · Wikimedia-Incident, Operations
tstarling renamed T232224: September 2019 DoS attacks [Public] from ERR_CONNECTION_TIMED_OUT on multiple WikiMedia sites to September 2019 DoS attack.
Sat, Sep 7, 5:27 AM · Wikimedia-Incident, Operations

Fri, Sep 6

tstarling committed rERELe280a5d2616f: Fix the more obvious XSS vulnerabilities and the CI test failures (authored by tstarling).
Fix the more obvious XSS vulnerabilities and the CI test failures
Fri, Sep 6, 8:01 PM
tstarling committed rEOAUb64cecda8588: Make OAuth work with the REST API (authored by tstarling).
Make OAuth work with the REST API
Fri, Sep 6, 1:13 AM

Thu, Sep 5

tstarling committed rERELe19f02d35e09: Use $wgRequest->getVal('title') instead of $_GET['title'] (authored by tstarling).
Use $wgRequest->getVal('title') instead of $_GET['title']
Thu, Sep 5, 7:23 AM
tstarling added a comment to T230473: Bug: <math> output broken (when parsed with Parsoid/PHP).

@tstarling this is exactly the right way of thinking. However, I do not understand why the titles are a sperate batch?

Thu, Sep 5, 6:18 AM · Parsoid-PHP

Wed, Sep 4

tstarling added a comment to T231598: Compose Count Queries.

Does it have to be multiple queries? It seems like it would be most efficient to do it all in a single table scan.

Wed, Sep 4, 12:24 PM · DBA, Core Platform Team Workboards (Green), CPT Initiatives (Core REST API in PHP)
tstarling moved T230914: User authentication with OAuth 1.0 from Ready to Waiting for Review on the Core Platform Team Workboards (Green) board.
Wed, Sep 4, 12:06 PM · Core Platform Team Workboards (Green), Story, CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T230473: Bug: <math> output broken (when parsed with Parsoid/PHP).

My idea is to add a batch option to Parser::setHook(), and a third type of strip marker to StripState. When registering a batched tag, the extension can also specify a collection name, allowing tags of different names to be collected into the same batch. Parser::extensionSubstitution() will select the marker type based on the batch option of the tag. To StripState we will add addBatched() and unstripBatched(), and unstripBoth() will become a deprecated alias to unstripAll(). unstripBatched() will gather batch-type markers from the input string and call each extension with the resulting array. The return value from the extension will be an array mapping each marker to its expanded HTML, and unstripBatched() will be responsible for making a second pass over the input string, inserting these HTML fragments.

Wed, Sep 4, 4:04 AM · Parsoid-PHP
tstarling updated subscribers of T230473: Bug: <math> output broken (when parsed with Parsoid/PHP).

@Physikerwelt The problem is, the parser uses unstripBoth() to remove markers for various good reasons, and if Math doesn't respond to those requests, the markers will leak through to the output. Pretty much every call to unstripBoth() in Parser.php is a bug causing exposure of these Math strip markers. For example == <math>1 \over 2</math> == generates the HTML

Wed, Sep 4, 2:01 AM · Parsoid-PHP
tstarling added a comment to T230473: Bug: <math> output broken (when parsed with Parsoid/PHP).

This is the Math extension being weird. It inserts its own special variety of strip marker, saves the details to a private static variable, and then replaces the markers in a ParserAfterTidy hook. That hook is not called from Parser::recursiveTagParseFully() despite that function being documented as returning "HTML that is safe for output".

Wed, Sep 4, 1:36 AM · Parsoid-PHP
tstarling updated subscribers of T230979: CR+2 on MediaWiki for Aryeh Gregor (aka Simetrical).

I must also note that the policy as currently written references to https://gerrit.wikimedia.org/r/#/admin/groups/119,members as the group of people able to "resolve the request" (sic), which is not true as Gerrit Managers do not have the ability to add or remove people from mediawiki. Looks like the link has to be changed either to point to https://gerrit.wikimedia.org/r/#/admin/groups/1,members or https://tools.wmflabs.org/ldap/group/gerritadmin (cc. @tstarling ).

Wed, Sep 4, 12:08 AM · MediaWiki-Gerrit-Group-Requests
tstarling closed T230979: CR+2 on MediaWiki for Aryeh Gregor (aka Simetrical) as Resolved.

There's no mediawiki LDAP group, and the policy does not refer to such a group. I added Aryeh to the mediawiki group in Gerrit.

Wed, Sep 4, 12:06 AM · MediaWiki-Gerrit-Group-Requests
tstarling added a comment to T228342: Define criteria for setting explicit PHP support target for MediaWiki.

Note that we've done minor version bumps e.g. from 7.0.0 to 7.0.13 for T209423 due to PHP bug https://bugs.php.net/bug.php?id=66862; would those kind of minor version changes be in scope for your 18 month guideline?

Wed, Sep 4, 12:02 AM · TechCom-RFC (TechCom-Approved), MediaWiki-General, PHP 7.3 support, PHP 7.2 support, PHP 7.1 support, PHP 7.0 support

Thu, Aug 29

tstarling closed T209448: Profiler subclass for excimer as Resolved.

This was done a long time ago.

Thu, Aug 29, 11:45 PM · Excimer
tstarling added a comment to T231335: Excimer: Use class/method instead of filepath for attributing closures.

Excimer should show both the filename and line number of the start of the closure declaration, not just the filename. This is the same information as is available with debug_backtrace(). To map that to a class name, we would have to iterate through all defined classes, checking the filename and line number range, until a match is found.

Thu, Aug 29, 9:15 AM · Core Platform Team, Performance-Team (Radar), Excimer
tstarling added a comment to T228342: Define criteria for setting explicit PHP support target for MediaWiki.

I'm sympathetic to @Legoktm's request that we avoid complicating CI and backports by regularly changing the supported PHP version. At most we can change our minimum supported PHP version once for every major PHP release, but in the past we have not updated it as often as that. PHP releases are approximately annually, so at most we could update the minimum PHP version once every two major MW releases. This ticket proposes PHP 7.2 for MW 1.34, when it was last updated from PHP 7.0 for MW 1.31, so a gap of three major MW releases. I think that's a reasonable compromise, and a minimum update period of 18 months and 3 major MW releases seems like a reasonable policy going forwards.

Thu, Aug 29, 12:40 AM · TechCom-RFC (TechCom-Approved), MediaWiki-General, PHP 7.3 support, PHP 7.2 support, PHP 7.1 support, PHP 7.0 support

Wed, Aug 28

tstarling added a comment to T228342: Define criteria for setting explicit PHP support target for MediaWiki.

That's another thing, the task description lacks any mention of WMF production, which has often been the sole reason for continuing to support a given PHP version.

Wed, Aug 28, 9:26 PM · TechCom-RFC (TechCom-Approved), MediaWiki-General, PHP 7.3 support, PHP 7.2 support, PHP 7.1 support, PHP 7.0 support
tstarling added a comment to T228342: Define criteria for setting explicit PHP support target for MediaWiki.

I'd discourage this line of thinking, a little. Changing versions only at LTS releases means

  • (a) much increased testing stress for the LTS release — risking the timing and confidence of the release and increasing the likelihood that we need to do an emergency follow-up point release — which undermines the confidence sysadmins would have in such a release,
Wed, Aug 28, 9:06 PM · TechCom-RFC (TechCom-Approved), MediaWiki-General, PHP 7.3 support, PHP 7.2 support, PHP 7.1 support, PHP 7.0 support
tstarling added a comment to T188375: castor rsync's taking 3-5 minutes for mwgate-npm jobs.

I talked with @Legoktm about the possibility of removing the npm cache from this rsync'd cache directory, and instead using a central network service for individual packages, for example using local-npm.

Wed, Aug 28, 12:31 AM · Continuous-Integration-Infrastructure

Tue, Aug 27

tstarling updated subscribers of T228795: Investigate API and client architecture requirements for edit history, diffs and related user actions.

@Milimetric might have some ideas about good ways to achieve this using the analytics infrastructure.

Tue, Aug 27, 6:55 AM · iOS-app-v6.5-Squid-On-A-Tandem-Bike, Spike, Wikipedia-iOS-App-Backlog
tstarling added a comment to T228794: Investigate technical needs for native mobile diffs, including supporting APIs.

We can have a REST API which delivers a JSON diff produced by @Tsevener's patch running on the server side, if that's what you need. For performance reasons, structured data isn't available in PHP, but as @Tsevener has evidently noticed, structured data is available in C++.

Tue, Aug 27, 4:41 AM · iOS-app-v6.5-Squid-On-A-Tandem-Bike, Spike, Wikipedia-iOS-App-Backlog

Mon, Aug 26

tstarling added a comment to T226598: REST API i18n.

That's why the plaintext type was omitted from the WIP patch, although I see you wrote a Gerrit comment asking for it to be re-added. I can add both types, for future compatibility with T227447 etc., but add a comment to MessageFormatter stating that their behaviour is undefined.

Mon, Aug 26, 11:11 PM · MW-1.34-notes (1.34.0-wmf.21; 2019-09-03), Core Platform Team Workboards (Green), CPT Initiatives (Parsoid REST API in PHP (CDP2)), MediaWiki-REST-API, Services (watching)
tstarling added a comment to T226598: REST API i18n.

That strikes me as rather dangerous. The caller has no way to know whether the result is safe to be used as HTML.

Mon, Aug 26, 7:43 AM · MW-1.34-notes (1.34.0-wmf.21; 2019-09-03), Core Platform Team Workboards (Green), CPT Initiatives (Parsoid REST API in PHP (CDP2)), MediaWiki-REST-API, Services (watching)
tstarling added a comment to T230843: Contributor updates a page.

This is a long way short of being a functional edit API. Have a look at ApiEditPage, it has 27 parameters, this proposal has 4. And there are controller-level complexities which are not represented here, like edit conflicts and CAPTCHA delivery. I don't know why we need this module or what kind of client the contributor is supposed to be using. If the idea is to provide feature parity with ApiEditPage, the design should start by looking at it.

Mon, Aug 26, 6:56 AM · Core Platform Team Workboards (User Stories), Story, CPT Initiatives (Core REST API in PHP)
tstarling renamed T229663: REST: Reader gets a page from Reader gets a page to REST: Reader gets a page.
Mon, Aug 26, 6:39 AM · Core Platform Team Workboards (User Stories), MediaWiki-REST-API, Story, CPT Initiatives (Core REST API in PHP)
tstarling added a comment to T229663: REST: Reader gets a page.

Like @mobrovac I think it's very unclear what sort of reader would want a page as JSON, and what should be in the JSON. Is the user here really a reader, or are they an editor who wants to subsequently edit the page via T230843?

Mon, Aug 26, 6:38 AM · Core Platform Team Workboards (User Stories), MediaWiki-REST-API, Story, CPT Initiatives (Core REST API in PHP)
tstarling updated the task description for T226043: MediaWiki REST API router should handle HEAD requests.
Mon, Aug 26, 6:23 AM · MW-1.34-notes (1.34.0-wmf.23; 2019-09-17), MediaWiki-REST-API
tstarling added a comment to T229433: ResourceLoaderWikiModule.php: PHP Notice: Undefined index: .

This would probably be enough to make it successfully do a core dump in production:

Mon, Aug 26, 3:18 AM · Performance-Team, Wikimedia-production-error, MediaWiki-ResourceLoader
tstarling added a comment to T229433: ResourceLoaderWikiModule.php: PHP Notice: Undefined index: .

Only three instances in the logs in 5 days. Maybe it should throw an exception instead of just logging a warning, to avoid breaking the caches? I investigated whether it could be an instance of https://bugs.php.net/bug.php?id=78379 -- it seems unlikely, since I couldn't find anywhere where getDefinitionSummary() is cast to an object. I guess the most likely thing is that there is something wrong with the array, causing the assignment to be effectively ignored. A core dump would help to narrow it down.

Mon, Aug 26, 2:33 AM · Performance-Team, Wikimedia-production-error, MediaWiki-ResourceLoader

Aug 23 2019

tstarling added a comment to T230861: PHP 7.2 is very slow on an allocation-intensive benchmark.

Tracing of TagTk::__destruct() shows that tokens are freed as it goes, they're not the problem. Disabling the GC causes the root buffer to no longer be maintained, so after you re-enable it, it'll be empty and doing a manual GC won't do anything. But we don't need to do a manual GC run anyway, since as I've documented, the memory usage is the same with it disabled. The high memory usage is due to live, uncollectable objects.

Aug 23 2019, 9:02 AM · PHP 7.3 support, PHP 7.2 support, serviceops, Operations
tstarling added a comment to T230861: PHP 7.2 is very slow on an allocation-intensive benchmark.

PHP 7.3 rearranges the bitfield so that the address gets 20 bits. It's a binary compatibility break, and possibly a source compatibility break, and beyond the scope of what I wanted to do with a simple patch.

Aug 23 2019, 3:49 AM · PHP 7.3 support, PHP 7.2 support, serviceops, Operations
tstarling added a comment to T230861: PHP 7.2 is very slow on an allocation-intensive benchmark.

Fun fact: the "gc address" of an object is packed into 14 bits, so increasing GC_ROOT_BUFFER_MAX_ENTRIES beyond 16383 causes corruption of the status bits, breaking the whole GC. In my previous tests, the GC probably didn't run at all. So I'm glad I investigated this further and we didn't just apply this patch.

Aug 23 2019, 12:46 AM · PHP 7.3 support, PHP 7.2 support, serviceops, Operations

Aug 21 2019

tstarling added a comment to T230861: PHP 7.2 is very slow on an allocation-intensive benchmark.

Cherry pick is not exactly the right word, I'm just proposing a temporary hack so that it will maybe work, whereas PHP 7.3 does a proper job of it. If we definitely want to increase GC_ROOT_BUFFER_MAX_ENTRIES on 7.2, I should do some more work first to figure out what the tradeoffs are and what the ideal value is. Invoking the GC less often presumably means using more memory. How much more will depend on the test case.

Aug 21 2019, 1:07 PM · PHP 7.3 support, PHP 7.2 support, serviceops, Operations
tstarling added a comment to T230861: PHP 7.2 is very slow on an allocation-intensive benchmark.
diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c
index 47236a934e..890b3e00f7 100644
--- a/Zend/zend_gc.c
+++ b/Zend/zend_gc.c
@@ -73,7 +73,7 @@
 #include "zend_API.h"
Aug 21 2019, 6:27 AM · PHP 7.3 support, PHP 7.2 support, serviceops, Operations
tstarling created T230861: PHP 7.2 is very slow on an allocation-intensive benchmark.
Aug 21 2019, 6:23 AM · PHP 7.3 support, PHP 7.2 support, serviceops, Operations
tstarling added a comment to T230647: +2 for NavinoEvans on labs/tools/lsa.git.

+1

Aug 21 2019, 12:49 AM · Gerrit-Privilege-Requests
tstarling added a comment to T214618: Requesting repository ownership for /mediawiki/extensions/OdbcDatabase.

I think this should be approved.

Aug 21 2019, 12:46 AM · Gerrit-Privilege-Requests
tstarling updated subscribers of T212452: Requesting repository ownership : Push.

I see you haven't replied to code review comments which were eventually made by @Reedy . Do you still want this?

Aug 21 2019, 12:24 AM · Gerrit-Privilege-Requests
tstarling closed T228872: Request for Gerrit Manager permission as Resolved.

Done.

Aug 21 2019, 12:20 AM · Cleanup, Repository-Admins, Gerrit-Privilege-Requests
tstarling added a comment to T230492: Requesting SRE permissions to create Gerrit projects under operations/debs.

The "Create Project" permission is global, you can't give a group Create Project permissions on a particular path.

Aug 21 2019, 12:13 AM · Gerrit-Privilege-Requests

Aug 16 2019

tstarling added a comment to T217849: Remex needs documentation of how to use its API.

I made a start at https://www.mediawiki.org/wiki/RemexHtml.

Aug 16 2019, 5:57 AM · RemexHtml

Aug 6 2019

tstarling added a comment to T228346: PHP 7.2 garbage collector segfault.

I isolated it and made a small test case. Submitted https://bugs.php.net/bug.php?id=78379 .

Aug 6 2019, 5:45 AM · Patch-For-Review, Parsoid-PHP, PHP 7.2 support

Aug 5 2019

tstarling added a comment to T228346: PHP 7.2 garbage collector segfault.

I'm using AddressSanitizer now, instead of valgrind. To integrate AddressSanitizer with PHP, the environment variable USE_ZEND_ALLOC=0 must be set. Compile with CFLAGS='-fsanitize=address' LIBS='-lasan -ldl'. Garbage collection is part of the bug, and adding gc_collect_cycles() before and after the call to runTestInMode() helps to trigger it earlier.

Aug 5 2019, 5:44 AM · Patch-For-Review, Parsoid-PHP, PHP 7.2 support

Aug 2 2019

tstarling added a comment to T228346: PHP 7.2 garbage collector segfault.

Running under valgrind with gdbserver shows a crash in json_encode() while serializing a corrupt array that came from the DataBag. The backtrace indicates DOMDataUtils::storeDataAttribs(). So the DataBag was probably accessed after free due to a bug in the DOM extension. I patched Parsoid to store DOM-associated data in an SplObjectStorage instead, and there was no crash.

Aug 2 2019, 4:08 AM · Patch-For-Review, Parsoid-PHP, PHP 7.2 support
tstarling added a comment to T228346: PHP 7.2 garbage collector segfault.

I was able to reproduce this locally. As nikic says on GitHub, the patch cannot be meaningfully applied to 7.2. I suspect it's a double-free or use-after-free, and the exact consequences of that vary from version to version. With USE_ZEND_ALLOC=0, it aborts on exit with "corrupted double-linked list", which is a pretty sure sign that the allocator is not the root cause. I'll try to isolate it further.

Aug 2 2019, 2:03 AM · Patch-For-Review, Parsoid-PHP, PHP 7.2 support
tstarling added a comment to T229541: Javascript injection in edit summary on mobile site (CVE-2019-14807).

Patch reviewed, looks good to me.

Aug 2 2019, 12:50 AM · MW-1.34-notes (1.34.0-wmf.20; 2019-08-27), Vuln-XSS, MobileFrontend, Security

Aug 1 2019

tstarling reopened T229366: serialize(): "" returned as member variable from __sleep() but does not exist, a subtask of T220741: 1.34.0-wmf.16 deployment blockers, as Open.
Aug 1 2019, 12:02 AM · Release-Engineering-Team-TODO (201908), Release-Engineering-Team (Deployment services), Release, Train Deployments
tstarling reopened T229366: serialize(): "" returned as member variable from __sleep() but does not exist as "Open".

There are more log entries now from mediawikiwiki. Inspecting them in eval.php shows that they are due to cache entries saved before the fix was deployed. They have ParserOutput::$speculativeRevIdUsed as the unknown private member. Reducing priority since there is no new parser cache corruption and no user-visible impact.

Aug 1 2019, 12:02 AM · Performance-Team (Radar), MW-1.34-notes (1.34.0-wmf.17; 2019-08-06), Core Platform Team Workboards (Clinic Duty Team), MediaWiki-Cache, Wikimedia-production-error

Jul 31 2019

tstarling assigned T224443: PHP error: "Undefined index: subtitle" from SpecialCollection to Pchelolo.
Jul 31 2019, 11:38 PM · MW-1.34-notes (1.34.0-wmf.17; 2019-08-06), Core Platform Team Workboards (Clinic Duty Team), Collection, Wikimedia-production-error
tstarling added a comment to T201749: Config script still sets "en_US.utf8" to "$wgShellLocale" for MW 1.30+.

In what way does it "mess up all files"? What is the impact?

Jul 31 2019, 11:19 PM · Core Platform Team Workboards (Clinic Duty Team), MW-1.34-release, MediaWiki-Installer, MediaWiki-Configuration
tstarling assigned T228763: stubs are produced with xml:space="preserve" in the text tag; this is new behavior for the July 20th run of the xml/sql dumps to daniel.
Jul 31 2019, 11:14 PM · CPT Initiatives (MCR), Core Platform Team Workboards (Clinic Duty Team), Dumps-Generation