you need to fix travis CI errors

Reading the JApplicationWebTest - it's actually testing for the issue this PR is meant to resolve.

The failure is on line 1321 of the test, which is within testSetHeader:

    public function testSetHeader()
    {
        // Fill the header body with an arbitrary value.
        TestReflection::setValue(
            $this->class,
            'response',
            (object) array(
                'cachable' => null,
                'headers' => array(
                    array('name' => 'foo', 'value' => 'bar'),
                ),
                'body' => null,
            )
        );
        $this->class->setHeader('foo', 'car');
        $this->assertEquals(
            array(
                array('name' => 'foo', 'value' => 'bar'),
                array('name' => 'foo', 'value' => 'car')
            ),
            TestReflection::getValue($this->class, 'response')->headers
        );
        $this->class->setHeader('foo', 'car', true);
        $this->assertEquals(
            array(
                array('name' => 'foo', 'value' => 'car')
            ),
            TestReflection::getValue($this->class, 'response')->headers
        );
    }

Notice, the 2nd time the header is set, a duplicate header (foo) exists. The way headers are set, by name, means that only the 2nd header will be added to the resulting output - effectively replacing the original entry without using $replace=true!

This is a bad test, it was written to support an error. The second setHeader should fail because a duplicate name exists without $replace=true.

@mbabker can you help here?

Anyway, you also have a lot of code style errors.
For performance reasons, the code style test and consequent errors only appear in php 5.6 test
See https://travis-ci.org/joomla/joomla-cms/jobs/122080816#L1323

OK, code style errors resolved (including one that was present in the existing code)

ok. seems fine.

BTW i think the unit test failing is this:
https://github.com/stutteringp0et/joomla-cms/blob/patch-1/tests/unit/suites/libraries/joomla/application/JApplicationWebTest.php#L1299

That's what I said earlier...

within testSetHeader

In JApplicationWeb::header, when headers are set - they're set with $replace=true 4 out of 5 times (the default) with the exception of the status header. So it becomes even more important that $replace be enforced in setHeader, as the only other place it gets enforced isn't ever being used beyond the status code.

JApplicationWebTest needs repair before this issue can be resolved.

you can repair it ...

Just edit in https://github.com/stutteringp0et/joomla-cms/blob/patch-1/tests/unit/suites/libraries/joomla/application/JApplicationWebTest.php#L1299

working on that now....Do I reference the commit here? These must be fixed in correct order.

just directly edit the file in this link https://github.com/stutteringp0et/joomla-cms/blob/patch-1/tests/unit/suites/libraries/joomla/application/JApplicationWebTest.php#L1299

If you edit in this link it will automatically changed in this PR

no, don't do another PR for that

oops...sorry

just directly edit the file in this link https://github.com/stutteringp0et/joomla-cms/blob/patch-1/tests/unit/suites/libraries/joomla/application/JApplicationWebTest.php#L1299

If you edit in this link it will automatically changed in this PR

no problem just close the other PR

OK, fixed it

i have some questions regarding this.

• The first is the test instructions (they could be more clear). for what i understand we have two do this:

  1. Install the plugin you attach and enable it
  2. Go to frontend and check the HTTP headers generated
  3. ...

• The second question is more conceptual.

  Will this remove the possibility of adding several HTTP headers with same designation?
  Remember, this behavior is allowed by HTTP protocol. See http://stackoverflow.com/questions/4371328/are-duplicate-http-response-headers-acceptable

Line 955 of JApplicationWeb defines the header() method, which defaults to $replace=true. The only time the header method is used where $replace is altered (set to null - line 691) is for the status header, but header() only responds to FALSE, so null doesn't do anything on line 691. So any duplicated header gets replaced anyway.

Sorry, yes you're right about the test. Install and run the plugin and check the Expires header. Alter JApplicationWeb and check the header again.

So the test can be just add to protostar index.php ...

JFactory::getApplication()->setHeader('Test', 'aaa');
JFactory::getApplication()->setHeader('Test', 'bbb');
JFactory::getApplication()->setHeader('Test 2', 'ccc');
JFactory::getApplication()->setHeader('Test 2', 'ddd', true);

Result:

Before patch

Test: bbb
Test 2: ddd

Affter patch

Test: aaa
Test 2: ddd

For what i understand, should be

Test: aaa, bbb
Test 2: ddd

I'm not 100% sure it should be like this tought.

"should be" if multiple values are required, but that would require extensive rewrites to accomplish.

This might be a better test, as it illustrates how setHeader $replace is pointless before the patch. $replace defaults to false, but ccc replaces ddd anyway:

JFactory::getApplication()->setHeader('Test', 'aaa');
JFactory::getApplication()->setHeader('Test', 'bbb');
JFactory::getApplication()->setHeader('Test 2', 'ddd', true)
JFactory::getApplication()->setHeader('Test 2', 'ccc')

Before patch:

Test: bbb
Test 2: ccc

After patch:

Test: aaa
Test 2: ddd

I should have looked closer at your aaa,bbb test - you got it

IMO

JFactory::getApplication()->setHeader('Test 2', 'ccc');
JFactory::getApplication()->setHeader('Test 2', 'ddd', true);
JFactory::getApplication()->setHeader('Test 2', 'eee');

should return Test 2: ddd, eee

JFactory::getApplication()->setHeader('Test 2', 'ccc', true);
JFactory::getApplication()->setHeader('Test 2', 'ddd');
JFactory::getApplication()->setHeader('Test 2', 'eee');

should return Test 2: ccc, ddd, eee

JFactory::getApplication()->setHeader('Test 2', 'ccc');
JFactory::getApplication()->setHeader('Test 2', 'ddd');
JFactory::getApplication()->setHeader('Test 2', 'eee', true);

should return Test 2: eee

JFactory::getApplication()->setHeader('Test 2', 'ccc');
JFactory::getApplication()->setHeader('Test 2', 'ddd');
JFactory::getApplication()->setHeader('Test 2', 'eee');

should return Test 2: ccc, ddd, eee

The order in which you execute the actions is important.

Also, i'm not sure how B/C would a change like this be.

That requires a major rewrite, as Joomla headers don't accommodate multiple values currently. They ALWAYS replace.

JApplicationWeb line 959

In order to accomplish this, the values would need to be collected before being sent in the response. And not all headers will accommodate this. Several headers can, but Expires header cannot have duplicates http://tools.ietf.org/html/rfc7234

An Expires header already contains a comma in the date - so a comma separated list wouldn't work

In order to accommodate what it "should" return (which I would agree with, if anything in Joomla required it) - there would need to be a list of which headers can have multiple values and which cannot.

The headers array stored in JApplicationWeb::setHeader would need to respect $replace and also have another parameter to allow for multiple values, verifying that duplicates of that header are allowed. Maybe something like

replace: $replace = true
duplicate: $replace = false

JApplicationWeb::sendHeaders would need to aggregate duplicates before sending them to JApplicationWeb::header

JApplicationWeb::header would need to check for an array value, and implode(',',$value) before sending the header...

Not impossible, but I wouldn't want to be the guy to read all of the RFCs to figure out which headers can be multiple and which cannot

The RFC multiple header list isn't all that impossible - working on it now.

Headers defined in RFC 7230-7237 which are designated as allowing multiple values are as follows:

Transfer-Encoding
Trailer
Via
Connection
Upgrade
Content-Encoding
Content-Language
Accept
Accept-Charset
Accept-Encoding
Accept-Language
Vary
Allow
If-Match
If-None-Match
Cache-Control
Pragma
Warning
WWW-Authenticate
Proxy-Authenticate

The only question I have is, if I were to implement this change - what classification would I give to headers not defined in the RFCs? Would I consider them to be multiple capable or not?

the first question you should make before doing anything is: would a change like this be B/C?
Note that if not, it only can be for 4.0

I could do it with B/C

So what's the suggestion for headers not defined in RFC 7230-7237 - multiple or not?

I'm leaning toward multiple

the options are: multiple headers or ...?

Either the header allows multiple values, or it doesn't. I've got a list of ones that allow multiples, and I'm working on a list that don't allow multiples.

if doesn't allow multiple values, what happens? It replaces? does nothing?

That all depends on how setHeader is called.

If the header allows multiple, and setHeader is called with:
$replace=true, then it's replaced.
$replace=false, it's added to the array

If the header does not allow multiple, and setHeader is called with:
$replace=true, then it's replaced
$replace=false and another already exists, it's ignored

If the header is undefined in the RFCs, it's either treated as a multiple or a single - I just don't know which it should be.... Like I said, I'm leaning toward multiple

OK, I've done it @andrepereiradasilva

Your ideal result, comma separated values where applicable. And my ideal result, $replace behaves as expected. And the really real ideal - it's all B/C

Do I close this PR and open a new one? It's still the same two files, no more.

i think you should make it in this PR.

also if you have time please also update the test instructions in the PR descriptions for all possible scenarios so others can test.

OK, this is going to take me a while to clean up

OK, I'm ready to begin making the changes - I'll comment again when it passes Travis CI

All I have are formatting errors left....

Bah, formatting errors are murder!

OK, here's the new test procedure. I like Andre's method of altering your template index.php file, so let's go with that.

The following HTTP headers are defined in RFC 7230-7237 to allow only one value: HTTP status, Content-Length, Host, Content-Type, Content-Location, Date, Location, Retry-After, Server, Mime-Version, Last-Modified, ETag, Accept-Ranges, Content-Range, Age, and Expires

Any other header will be allowed to create duplicates, and if duplicates are created - per RFC 7230-7, the duplicates are turned to a comma separated list.

I'm starting this off with an $app variable to bring my test line lengths down to something that won't wrap in this description.

$app = JFactory::getApplication();

First test shows that setting a duplicate single value header like Expires without $replace does not replace the value as was the previous behavior.

$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT',true); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT'); // 10 minutes

Result: Expires 1 hour future from the Date header.
(previous behavior would have the 15 minute future header, and this is the reason I made this change - because JApplicationWeb::respond sets it to now+900 seconds without $replace=true, replacing any previous value)

Now, let's reverse the $replace value

$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT'); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT',true); // 10 minutes

Result: Expires 10 minutes future from the Date header.
(previous behavior would have the 15 minute future header, and this is the reason I made this change - because JApplicationWeb::respond sets it to now+900 seconds without $replace=true, replacing any previous value)

Next we repeat the last test with no $replace value

$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT'); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT'); // 10 minutes

Result: Expires 1 hour future from the Date header.
(previous behavior would have the 15 minute future header, and this is the reason I made this change - because JApplicationWeb::respond sets it to now+900 seconds without $replace=true, replacing any previous value)

Our next tests relate to a made-up header. All headers not in the single value list are considered to allow multiple values

$app->setHeader('Test-Header','aaa');
$app->setHeader('Test-Header','bbb');
$app->setHeader('Test-Header','ccc');

Result: Test-Header value is "aaa, bbb, ccc"
(previous behavior would have a value of "ccc")

Next test issues $replace = true in the middle, losing one of the values

$app->setHeader('Test-Header','aaa');
$app->setHeader('Test-Header','bbb',true);
$app->setHeader('Test-Header','ccc');

Result: Test-Header value is "bbb, ccc"
(previous behavior would have a value of "ccc")

Next test issues $replace = true on the last value, losing the other entries

$app->setHeader('Test-Header','aaa');
$app->setHeader('Test-Header','bbb');
$app->setHeader('Test-Header','ccc',true);

Result: Test-Header value is "ccc"
(previous behavior would have a value of "ccc")

First test:

$app = JFactory::getApplication();

$app->setHeader('Test-Header','aaa');
$app->setHeader('Test-Header','bbb');
$app->setHeader('Test-Header','ccc');

$app->setHeader('Test-Header 2','aaa');
$app->setHeader('Test-Header 2','bbb',true);
$app->setHeader('Test-Header 2','ccc');

$app->setHeader('Test-Header 3','aaa');
$app->setHeader('Test-Header 3','bbb');
$app->setHeader('Test-Header 3','ccc',true);

Result:

// Before patch
Test-header: ccc
Test-header 2: ccc
Test-header 3: ccc

// After patch
Test-header: aaa, bbb, ccc
Test-header 2: bbb, ccc
Test-header 3: ccc

So this behaves as intended.

Will test the expires later.

Test with expires = problem

$app = JFactory::getApplication();
$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date

$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT',true); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT'); // 10 minutes

// Before patch: Expires: NOW + 15 minutes
// After patch: Expires: NOW + 16 minutes and 40 seconds

$app = JFactory::getApplication();
$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date

$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT'); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT',true); // 10 minutes

// Before patch: Expires: NOW + 15 minutes
// After patch: Expires: NOW + 16 minutes and 40 seconds

$app = JFactory::getApplication();
$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date

$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT'); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT'); // 10 minutes

// Before patch: Expires: NOW + 15 minutes
// After patch: Expires: NOW + 15 minutes

So, since this is not the intended behaviour, tested unsuccessfully.
But it can be my server conf too, when i do nothing i have an expires of NOW + 16 minutes and 40 seconds also.

As described before, the default expires when allowCache(true) is 15 minutes. I don't know where the additional 1 minute 40 seconds comes from though. I didn't manipulate the respond method (that's where the 15 minutes/900 seconds is introduced)

A discrepancy of a second or two between the Date and Expires header is explainable because Date is set by the server when the first header is sent and lots of things happen between that and Expires being set - but not 1 minute 40 seconds...unless your server runs really Really REALLY slow.

An example:

<?php
$now = gmdate('D, d M Y H:i:s', time()) . ' GMT';
sleep(30);
$exp = gmdate('D, d M Y H:i:s', time()) . ' GMT';
header('Expires: '.$exp);
echo '$now: '.$now; // should be nearly identical to the Date header
echo '<br />';
echo '$exp: '.$exp; // should be 30 seconds future of the Date header

This will be 30 seconds between Date and Expires.

I don't understand why you're not getting the Expires value that's being set.... It works as expected on my system. I'll try a fresh install and see what happens.

Fresh install - works as expected

maybe there is something with my server. i will check later when i have time.

Hahah, as long as this keeps sitting here - I'll just keep making performance improvements (just to the two methods I've been working on). This last commit removed 2 foreach loops that I wasn't pleased about adding at the first place.

I've got more formatting errors to resolve - I'll fix it up tonight and leave it for testing

@stutteringp0et is the first post description and test instructions updated with your latest changes?

@brianteeman please remove the "Unit/System Tests" label since now it doesn't make changes in the unit tests

@andrepereiradasilva yes, I updated the instructions in the initial post.

@stutteringp0et one thing how do i unset an header that has already been setted before. Is there a way?

for instance something like this doesn't work, right?

$app->setHeader('Test-Header', null, true); 

To more clear this is waht i'm trying to do to make tests easier

$app = JFactory::getApplication();
$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date
$testResults = array();
$datePlusOneHour = gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT';
$datePlusTenMinutes = gmdate('D, d M Y H:i:s', time() + 600) . ' GMT';

// Do tests
$app->setHeader('Expires', $datePlusOneHour, true); // 1 hour
$app->setHeader('Expires', $datePlusTenMinutes); // 10 minutes
$testResults['test1'] = $app->getHeaders();
$app->setHeader('Expires', null, true); // reset

$app->setHeader('Expires', $datePlusOneHour); // 1 hour
$app->setHeader('Expires', $datePlusTenMinutes, true); // 10 minutes
$testResults['test2'] = $app->getHeaders();
$app->setHeader('Expires', null, true); // reset

$app->setHeader('Expires', $datePlusOneHour); // 1 hour
$app->setHeader('Expires', $datePlusTenMinutes); // 10 minutes
$testResults['test3'] = $app->getHeaders();
$app->setHeader('Expires', null, true); // reset

$app->setHeader('Test-Header', 'aaa');
$app->setHeader('Test-Header', 'bbb');
$app->setHeader('Test-Header', 'ccc');
$testResults['test4'] = $app->getHeaders();
$app->setHeader('Test-Header', null, true); // reset

$app->setHeader('Test-Header', 'aaa');
$app->setHeader('Test-Header', 'bbb', true);
$app->setHeader('Test-Header', 'ccc');
$testResults['test5'] = $app->getHeaders();
$app->setHeader('Test-Header', null, true); // reset

$app->setHeader('Test-Header', 'aaa');
$app->setHeader('Test-Header', 'bbb');
$app->setHeader('Test-Header', 'ccc', true);
$testResults['test6'] = $app->getHeaders();
$app->setHeader('Test-Header', null, true); // reset

// Show test results
print_r($testResults);

I don't believe there ever was a facility to remove a header once set - they could only be added or replaced. It's not hard to accomplish, and I'm not opposed to adding it if you think it's necessary - but it isn't something that's being done now.

Edit: just tested pre-patch and setHeader('name',null,true) just creates an empty header (header with no value), so the method never removed on a null value.

I can make that happen if you think it's necessary.

Remove on null was really easy to accomplish, and I've got that ready to add if you think it's necessary.

actually i think it would be a good feature allowing to remove an already setted header.

If you could do it, it would be great.

Done, waiting on Travis

DARN YOU FORMATTING ERRORS - DARN YOU ALL TO HECK!

OK, it's done now, passed tests and your test now works as expected on my test server

ok will test.

this is my result

Before patch

Server date: 17:34:30 GMT

Array
(
    [test1] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 18:34:30 GMT
                )
            [1] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 17:44:30 GMT
                )
        )
    [test2] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 17:44:30 GMT
                )
        )
    [test3] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => 
                )
            [1] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 18:34:30 GMT
                )
            [2] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 17:44:30 GMT
                )
        )
    [test4] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => 
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => aaa
                )
            [2] => Array
                (
                    [name] => Test-Header
                    [value] => bbb
                )
            [3] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
    [test5] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => 
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => bbb
                )
            [2] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
    [test6] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => 
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
)

After patch

Server date: 17:37:40 GMT

Array
(
    [test1] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 18:37:40 GMT
                )
        )
    [test2] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 17:47:40 GMT
                )
        )
    [test3] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 18:37:40 GMT
                )
        )
    [test4] => Array
        (
            [0] => Array
                (
                    [name] => Test-Header
                    [value] => aaa
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => bbb
                )
            [2] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
    [test5] => Array
        (
            [0] => Array
                (
                    [name] => Test-Header
                    [value] => bbb
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
    [test6] => Array
        (
            [0] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
)

That looks right to me

I have tested this item successfully on 3ee123a

besides the tests above also checked each individual test generated http header.

This needs another human test to be completed, correct?

yes

What can I do to get someone else to test this?

@stutteringp0et I am going to see if I can find some time to test this.

Thank you @roland-d

I have tested this item ✅ successfully on 3ee123a

> First test shows that setting a duplicate single value header like Expires without $replace does not replace the value as was the previous behavior.
This works as expected, the expire header of 1 hour is active

Second test was to reverse the $replace value in case multiple times the header is set
This works as expected, the expire header of 10 minutes is active

Third test is not to use the $replace value
This works as expected because the first set value of 1 hour is active

Multiple values for a header value that allows it
This works as expected because I get the value aaa,bbb,ccc as expected

Multiple values for a header value that allows it but setting the $replace value on the second value
This works as expected because the first value is removed and only the 2nd and 3rd values are used as bbb,ccc

Multiple values for a header value that allows it but setting the $replace value on the third value
This works as expected because the first value is removed and only the 3rd value is used as ccc by replacing the first two values

All tests are successful.

Set to RTC as we have 2 successful tests