# User Details

User Since
Oct 15 2014, 8:27 PM (257 w, 5 d)
Roles
Availability
Available
LDAP User
Tim Starling
MediaWiki User

# 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.

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.

tstarling updated the task description for T231580: Implement GET Revision Comparison.
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"
},
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?

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.

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

Some additional considerations which apply to either interface style:

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.

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

Content negotiation version of this, for consideration:

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

• 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.
tstarling updated the task description for T231580: Implement GET Revision Comparison.
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.

# Tue, Sep 17

tstarling renamed T232485: RFC: Core REST API namespace and version from Namespace and version to Core REST API namespace and version.
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.

# 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.

# Sat, Sep 14

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]'.

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.

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

Continuing the core dump analysis:

# 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.

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.

# 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.

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.

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.

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.

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.

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.

tstarling added a comment to T231347: Curator compares revisions.

I renamed version to revision, as discussed.

tstarling renamed T231347: Curator compares revisions from Curator compares versions to Curator compares revisions.
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.

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.

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.

# 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.

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.

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

# Jul 31 2019

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