WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 2 years ago

#7968 closed enhancement (maybelater)

Use "wp_die" template message instead of plain message when Atom is disabled.

Reported by: Simek Owned by: westi
Milestone: Priority: lowest
Severity: trivial Version: 2.7
Component: AtomPub Keywords:
Focuses: Cc:

Description

Use "wp_die" template message instead of plain message when Atom is disabled.

Attachments (2)

wp_app.die.patch (647 bytes) - added by Simek 5 years ago.
7968.r10258.diff (485 bytes) - added by jacobsantos 5 years ago.
Based off of r10258

Download all attachments as: .zip

Change History (16)

Simek5 years ago

comment:1 DD325 years ago

The AtomServer::forbidden() function is where you should probably do that.

wp_die($reason, __('Atom Feed'), array('response' => 403) );

bundled inside that function would take care of it?

comment:2 westi5 years ago

  • Cc josephscott added
  • Keywords needs-patch added; patch removed
  • Owner changed from anonymous to westi
  • Status changed from new to assigned

Indeed if we are to wp_die it should be inside forbidden.

We also need to consider the other error cases here and how this change will possibly affect clients.

comment:3 ryan5 years ago

  • Milestone changed from 2.7 to 2.8

jacobsantos5 years ago

Based off of r10258

comment:4 jacobsantos5 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:5 westi5 years ago

josephscott - is this change likely to break APP clients or is it safe to commit?

comment:6 ryan5 years ago

  • Component changed from General to AtomPub

comment:7 Denis-de-Bernardy5 years ago

  • Keywords reporter-feedback added
  • Milestone changed from 2.8 to Future Release

not safe imo. moving to future release, as this really needs some testing.

comment:8 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9

comment:9 hakre5 years ago

  • Keywords tested added; needs-testing reporter-feedback removed

Tested the patch with various clients over HTTP which is the prefered tranport protocoll. Looks very good, the Status Header is transmitted as it must be, it gives a 403 forbidden message. Looks all perfectly well and to assume that this will create incompabilities must be a myth.

comment:10 follow-up: westi5 years ago

  • Keywords has-patch tested removed
  • Milestone changed from 2.9 to Future Release

I discussed this with the client developers on the wp-xmlrpc list and we feel that it would be better to have a parsable xml response here so that clients can parse it out.

However, there doesn't appear to be any provision for this in the APP RFC - there is no detail spec on error responses :-(

Moving it to future for now until we have a good workable patch in that direction.

comment:11 in reply to: ↑ 10 hakre5 years ago

  • Keywords has-patch tested added
  • Milestone changed from Future Release to 2.9

Replying to westi:

I discussed this with the client developers on the wp-xmlrpc list and we feel that it would be better to have a parsable xml response here so that clients can parse it out.

Thanks for sharing your feelings but I doubt that this is in the scope of the ticket, so let's stick with the basics.

However, there doesn't appear to be any provision for this in the APP RFC - there is no detail spec on error responses :-(

Well, the RFC I found (rfc5023) is perfectly clear about the issue, I wonder where you both might have looked:

5.5.  Use of HTTP Response Codes

   The Atom Protocol uses the response status codes defined in HTTP to
   indicate the success or failure of an operation.  Consult the HTTP
   specification [RFC2616] for detailed definitions of each status code.

   Implementers are asked to note that according to the HTTP
   specification, HTTP 4xx and 5xx response entities SHOULD include a
   human-readable explanation of the error.

http://tools.ietf.org/html/rfc5023#section-5.5

Moving it to future for now until we have a good workable patch in that direction.

I guess you just made a mistake looking up the specs so I revert your changes. As stated above having a 403 with a human readable response (HTML) should be favored over XML. The patch looks perfectly well to me, it actually is the implementation of that RFC. Having an empty body in the response as of now is the actual bug.

comment:12 follow-up: westi5 years ago

  • Keywords has-patch tested removed
  • Milestone changed from 2.9 to Future Release

That is exactly the spec I read - and it is very vague.

I also read a lot more information on this - an error response format was discussed at committee stage but did not progress further.

What we have now is better than html for an APP client to use - ideally a well formed parse-able XML response seems the best route for an even friendlier client response.

comment:13 in reply to: ↑ 12 hakre4 years ago

Replying to westi:

That is exactly the spec I read - and it is very vague.

vague? Next to referring other RFCs it ...

I also read a lot more information on this - an error response format was discussed at committee stage but did not progress further.

(then there ain't one)

What we have now is better than html for an APP client to use - ideally a well formed parse-able XML response seems the best route for an even friendlier client response.

... was clearly written that not a machine but a human readable response should be given.

comment:14 westi2 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from accepted to closed

Closing this down as we haven't seen much call for it and I haven't seen a strong uptake of APP by client developers.

Note: See TracTickets for help on using tickets.