There are some PHP coding standards, that you need to follow
see here:
https://joomla.github.io/coding-standards/?coding-standards/chapters/php.md

This is my very first pull request, can you please tell me what am I missing in my code or done incorectly?

Thank you

should be

if ()
{
     ....
}
else
{
    ....
}

see travis report for all detail https://travis-ci.org/joomla/joomla-cms/jobs/183720152

and https://docs.joomla.org/Joomla_CodeSniffer for more info

@alex-equities see alex-equities#1

@alex-equities you also need to add clear test instructions and add the summary of the changes to the PR description.

A PR to be merged needs two valids tests (aside form the PR creator tests), than a cms mantainer makes it RTC, and finnaly another CS mantainer review it and merge it.

Thank you for all your response, I now have more understanding of what needs to be done for the next one. :)

I see now both checks are passed, what is going to happen next?

Thanks

Now we need 2 successful test....

On 14 Dec 2016 2:39 pm, "AlexP" notifications@github.com wrote:

I see now both checks are passed, what is going to happen next?

Thanks

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#13184 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AALFsex8zIjDhi8fidACs65fxEPZtPIMks5rH_GegaJpZM4LMNZX
.

and the tests are made by certain people?

Is allowed to everyone to test...

On 14 Dec 2016 2:55 pm, "AlexP" notifications@github.com wrote:

and the tests are made by certain people?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#13184 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AALFsbCcZc39GHvTZIyJZncVtRuWafNVks5rH_U_gaJpZM4LMNZX
.

Two individuals aside from the person who submits the pull request must test it to make sure it works correctly then someone with the ability to merge will review the patch and either merge it or make additional comments requesting changes or additional testing.

IIRC, altough i think it's not mandatory, the HTTP status codes should be the status code + status code reason text (not translated).

ex: 404 Not Found, 500 Internal Server Error

List https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
http://www.ietf.org/assignments/http-status-codes/http-status-codes.xml

@andrepereiradasilva, that is exactly what will happen with this update. Just show the 404 Not Found or 500 Internal Server Error, with no extra information

Example for some frontend url that does not exists:

In English:

• after patch: Status Code: 404
• before patch: Status Code: 404 Article not found

In Portuguese (as an example):

• after patch: Status Code: 404
• before patch: Status Code: 404 Artigo n%C3%A3o encontrado

It'd be great if we had something like Symfony's Response object (mainly the HTTP status stuff and methods like setStatusCode(); JApplicationWeb inlines a lot of what their Response object does but isn't as abstracted out as Symfony's code). This way we could just pass a code in and optional text and the API validate the data is correct and stick the correct text in place when it isn't provided. Right now you have to do all of that work yourself because the only thing we offer is setHeader().

that is weird but I'm getting -
before patch - Status Code:404 Article not found
after patch - Status Code:404 Not Found

even tried with broken user URL
before patch - Status Code:404 User not found
after patch - Status Code:404 Not Found

HTTP headers in my case.

before

HTTP/1.1 404 Artigo n%C3%A3o encontrado
Server: nginx
Date: Fri, 16 Dec 2016 15:30:26 GMT
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
Vary: Accept-Encoding
Cache-Control: no-cache
Content-Encoding: gzip

after

HTTP/1.1 404
Server: nginx
Date: Fri, 16 Dec 2016 15:29:35 GMT
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
Vary: Accept-Encoding
Cache-Control: no-cache
Content-Encoding: gzip

using chrome developer toolbar

JApplicationWeb doesn't set status texts when you don't give it one; its header APIs aren't that smart. Without having an API method to do it in Joomla core (see that Response::setStatusCode() method I alluded to before), any place setting a status header has to manually figure out what value to use.

You are actually correct, joomla returns number code only, but The Chrome itself added the text for 404, because it only needs the code.
I believe that is how you suppose to do it, passing the code Only, with no text.

here are 2 proves that I based on

1. Chrome it self shows the correct text if tried putting different status code manually (for example 301)
2. another example in PHP of how you set the redirect with just the code with no text
   header("location: https://go.here.com",TRUE,301);

The reason-phrase element exists for the sole purpose of providing a textual description associated with the numeric status code, mostly out of deference to earlier Internet application protocols that were more frequently used with interactive text clients. A client SHOULD ignore the reason-phrase content.

Source: RFC 7230 (3.1.2) https://tools.ietf.org/html/rfc7230#section-3.1.2

The reason phrases listed here are only recommendations -- they can be replaced by local equivalents without affecting the protocol.

Source: RFC 7231 (6.1) https://tools.ietf.org/html/rfc7231#section-6.1

So for what i understand, per rfc, they are optional and can be different (as joomla currently does).

Correct, so for the developer they will be visible with $error_reporting set to 'development' and for the public they will be standard messaging for http codes.

Great job getting to the bottom of that !

@andrepereiradasilva were you able to be one of the human testers? Where do I see that by the way?

Thanks

Anyone except the person submitting the PR can test it. It's assumed that person tested before submitting it ?

Sorry for wasn't being clear. I of course tested this 100% :) But I'm trying to understand how to see the status of my pull request. I assume it suppose to show somewhere that certain (not original submitter) tested it? and what is the next step, will it become approved or something and it will go into the next joomla update?

thank you.

i've followed another approach sometimes ago see #10949 for reference

@alikon - what happened to your pull request?

I have tested this item ✅ successfully on 59312a1

Works for me, thanks!

Thank you SniperSister for testing.

Final decision on that for @rdeutz

thank you very much for moving it along