? ? ? Pending

User tests: Successful: Unsuccessful:

avatar richard67
richard67
17 Mar 2021

Pull Request for Issue #32567 .

Summary of Changes

This pull request fixes the b/c issue with folders for files and images of com_media starting with something else than a letter a-z (upper or lower case) introduced with a security fix into the 3.9.25 release without introducing back the original security issue.

This is done by removing the check for the 1st character which was introduced to disable both full path disclosure by using a path like ./ or .\ and break out of the parent folder by using ../ or ..\ (depending on your server's OS).

The usage of ./ or .\ is handled instead by adding '.' to the exclude list for the parent folders:

// Exclude current folder '.' to be safe from full path disclosure
$exclude[] = '.';

The breakout from the parent folder by using something like ../ or ..\ (depending on your OS) is already handled by the Path::check($value) a few lines further down. I've added the comment in line 69 to make that more clear:

// Check if $value is a valid path, which includes not allowing to break out of the current path
try
{
Path::check($value);
}
catch (\Exception $e)
{
// When there is an exception in the check path this is not valid
return false;
}

In addition to this fix, this PR also fixes the splitting of the value into parts for the exclude list check.

When on a Windows server. you perfectly can set and later use a path to images or files separates with either a Linux or a Windows directory separator.

In case if your site has been migrated from a Windows server e.g. for local development to the final Linux server, you still might have a \ in a path, which would lead to false results when splitting the path for the exclude list check.

Therefore the change from $path = explode('/', $value); to $path = preg_split('/[\/\\\\]/', $value);.

Testing Instructions

  1. On a current staging or latest 3.9.x nightly build or 3.9.25, go to the media manager options in backend.
  2. Either in the "Path to Files Folder" or for "Path to Images Folder" field, enter a path that starts with something else than a letter a to Z or A to Z.
  3. Use the "Save" button.
    Result: You can't save the changes, you get a red error message shown for the particular field.
  4. Repeat steps 2 and 3 so you cover at least the following cases for both of these fields:
  • Hidden folder like eg .mysecretimages as root.
  • Root folder starting with a number, eg 1mississippi, 2mississippi, ...
  • Root folder starting with a Unicode character, e.g. Cyrillic Изображений, Greece Εικόνες, ...
  • Root folder starting with a symbol which is allowed for folder names on your server's OS, e.g. -dada, _gaga, ...

Result: Same as for step 3.

  1. Apply the patch of this PR.
  2. Repeat step 4, i.e. steps 2 and 3 at least for the cases mentioned in step 4.
    Result: You can save the changes.
  3. Enter a path starting with ./ or .\ and use the "Save" button.
    Result: You can't save the changes, you get a red error message shown for the particular field.
  4. Enter a path which starts with ../ or contains /../ (replace / by the actual directory separator of your server's OS) and use the "Save" button.
    Result: You can't save the changes, you get a red error message shown for the particular field.
  5. Enter a path which has one of the folders from the exclude list (see below) as the root folder, followed by a slash /, e.g. administrator/, and use the "Save" button.
    Result: You can't save the changes, you get a red error message shown for the particular field.
  6. Enter a path which has one of the folders from the exclude list (see below) as the root folder, followed by a backslash \, e.g. administrator\, and use the "Save" button.
    Result: You can't save the changes, you get a red error message shown for the particular field.

The exclude list for field "Path to Files Folder" can be found here:
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_media/config.xml#L41

The exclude list for field "Path to Images Folder" can be found here:
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_media/config.xml#L52

Actual result BEFORE applying this Pull Request

You can't save com_media settings when using a Path to Files Folder or a Path to Images Folder not starting with one of the letters a to z (case-insensitive).

This means eg the following is not allowed with 3.9.25 or current staging, which is a b/c break because it was allowed with 3.9.24 and older:

  • Hidden folder like eg .mysecretimages as root.
  • Root folder starting with a number.
  • Root folder starting a Unicode character, e.g. Cyrillic, Greece, ...
  • Root folder starting with a symbol which is allowed for folder names on your server's OS, e.g. - or _

Expected result AFTER applying this Pull Request

You can save com_media settings when using a Path to Files Folder or a Path to Images Folder not starting with one of the letters a to z (case-insensitive).

You still can't save when one of these paths starts with something like ./ or .\ on any OS or when it contains something like ../ on Linux or ..\ on Windows.

The check for the exclude list works with both kinds of directory separators /and \.

Documentation Changes Required

None.

avatar richard67 richard67 - open - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 17 Mar 2021
Category Libraries
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar richard67 richard67 - change - 17 Mar 2021
The description was changed
avatar richard67 richard67 - edited - 17 Mar 2021
avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

This is the perfect candidate for unit tests, being a validation rule, and being previously a security issue... Are there any unit tests that cover this ?

avatar richard67 richard67 - change - 17 Mar 2021
Labels Added: ?
avatar richard67
richard67 - comment - 17 Mar 2021

Are there any unit tests that cover this ?

If there were any unit tests for this I would have extended them by new test cases, but I haven't found any, and creating completely new tests is beyond my knowledge (up to now).

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

I will write the unit test. Please tell me, which of these do you believe should return false and which true:

./images
/images
.\images
../images
.../images
c:\images
\images
ftp://images
http://images
avatar zero-24
zero-24 - comment - 17 Mar 2021

I will write the unit test. Please tell me, which of these do you believe should return false and which true:

Great, i did go througth the stuff and added a few more.

./images => false
/images = true
.\images => false
../images => false
.../images => false
c:\images = true
\images => false
ftp://images => false
http://images => false
/media => false
/administrator => false
/4711images = true

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

media and administrator are covered in the won't fix issue here #32577

avatar zero-24
zero-24 - comment - 17 Mar 2021

Yes but that can be covered within the unit test too right?

avatar richard67
richard67 - comment - 17 Mar 2021

I will write the unit test. Please tell me, which of these do you believe should return false and which true:

Great, i did go througth the stuff and added a few more.

./images => false
/images = true
.\images => false
../images => false
.../images => false
c:\images = true
\images => false
ftp://images => false
http://images => false
/media => false
/administrator => false
/4711images = true

@zero-24 Some of the results depend on how Path::clean works, and this is different on Linux or Windows. Have you tested the above on both kinds of OS, or is your list just based on your assumption how it should be?

I have tested it on Linux and your list is wrong for some values.

P.S.: And shall absolute paths be allowed at this point? Currently they are not because $path[0]would be empty, and this is how it should be from my point of view.

I will not implement anything or merge anything into this PR which is based on assumptions only.

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

My unit test will be based on the current staging commit.

You can then quickly merge it - because all the tests pass.

If the tests no longer pass once you have merged staging into your branch, then you have created a b/c break from staging (or fixed the previous b/c that 3.9.25 caused :-) )

Im pushing in 2 mins...

avatar zero-24
zero-24 - comment - 17 Mar 2021

Have you tested the above on both kinds of OS, or is your list just based on your assumption how it should be?

based on my assumptions I might be wrong in some places.

avatar richard67
richard67 - comment - 17 Mar 2021

If the tests no longer pass once you have merged staging into your branch, then you have created a b/c break from staging (or fixed the previous b/c that 3.9.25 caused :-) )

They will break if you have a test for false being returned for the b/c that 3.9.25 caused?

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021
avatar richard67
richard67 - comment - 17 Mar 2021

@PhilETaylor You run them on Linux? Or Windows? Our testing environments run on Windows as far as I know, but the unit tests should also work on Linux. And there will be a differente due to current behaviour of Path::check.

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

This is why they need to be merged, because then they will get run.

I use a Mac.

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

Also if the Joomla test suites are only being run on windows, then /facepalm ...

avatar richard67
richard67 - comment - 17 Mar 2021

This is why they need to be merged, because then they will get run.

They can be run from command line, too, and as I said, some of your tests might fail on one or the other OS.

avatar richard67
richard67 - comment - 17 Mar 2021

@PhilETaylor Beside the above, what's the plan now? It needs to merge your PR before we can test and merge mine?

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

They can be run from command line, too,

They pass at the command line on my Mac, and it looks like the PR is passing unit testing too.

If the tests pass on one platform and fail on another, it would mean that the rule can return true when it should be false or false when it should be true... that's not a very good rule then.

Thats not the tests fault. Thats the rules fault :-)

It needs to merge your PR before we can test and merge mine?

Thats not my decision. I just submit PR's and Joomla does what Joomla wants with them. The unit test submitted passes at the time it was created against staging so it would be easy for a maintainer to merge it once approved by 2 people - or merged without testing it ;-)

I'll get a Joomla 4 one done too.

avatar richard67
richard67 - comment - 17 Mar 2021

If the tests pass on one platform and fail on another, it would mean that the rule can return true when it should be false or false when it should be true... that's not a very good rule then.

Thats not the tests fault. Thats the rules fault :-)

The rule used Path::clean before this PR and before the security fix, and it's that Path::clean which behaves different e.g. regarding the check for ..\ or ../.

So if that is a mistake, it doesn't come from this PR here.

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

Joomla 4 test here #32724 which passes based on the current state of 4.0-dev and can be changed/improved at a later date

avatar richard67
richard67 - comment - 17 Mar 2021

Also if the Joomla test suites are only being run on windows, then /facepalm ...

@PhilETaylor I think I was wrong with that and they are running on Linux, at least some of them.

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

AppVeyor is running windows.

https://ci.appveyor.com/project/release-joomla/joomla-cms/builds/38276109/job/lpq1nwygs34vr7qk?fullLog=true

Build started
2SET PATH=C:\Program Files\OpenSSL;C:\tools\php;%PATH%
3SET COMPOSER_NO_INTERACTION=1
4SET PHP=1
5SET ANSICON=121x90 (121x90)
6git clone -q https://github.com/joomla/joomla-cms.git C:\projects\joomla-cms
7git fetch -q origin +refs/pull/32723/merge:
8git checkout -qf FETCH_HEAD
9Running
.....

avatar richard67
richard67 - comment - 17 Mar 2021

AppVeyor is running windows.

Yes, I got it now. Thanks.

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

So to be clear. Is this still a b/c break? Are there any paths that used to work that will no longer work after this?

avatar richard67
richard67 - comment - 17 Mar 2021

So to be clear. Is this still a b/c break? Are there any paths that used to work that will no longer work after this?

No, this here should not be a b/c break so that it will make things not work which work without this PR, and if it is because I've missed something, then I have to fix it.

But it will fix the b/c break from 3.9.25.

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

applying this PR, against the unit tests (which are configured for staging, eg post 3.9.25 release) they fail:

PHPUnit 4.8.36 by Sebastian Bergmann and contributors.
Warning:	The Xdebug extension is not loaded
		No code coverage will be generated.

.F...........FFFFFF..FF

Time: 78 ms, Memory: 8.00MB

There were 9 failures:

1) FilePathRuleTest::testRule with data set #1 (false, SimpleXMLElement Object (...), '.images')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

2) FilePathRuleTest::testRule with data set #13 (false, SimpleXMLElement Object (...), '4711images')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

3) FilePathRuleTest::testRule with data set #14 (false, SimpleXMLElement Object (...), '1')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

4) FilePathRuleTest::testRule with data set #15 (false, SimpleXMLElement Object (...), '_')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

5) FilePathRuleTest::testRule with data set #16 (false, SimpleXMLElement Object (...), '*')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

6) FilePathRuleTest::testRule with data set #17 (false, SimpleXMLElement Object (...), '%')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

7) FilePathRuleTest::testRule with data set #18 (false, SimpleXMLElement Object (...), '://foo')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

8) FilePathRuleTest::testRule with data set #21 (false, SimpleXMLElement Object (...), 'Εικόνες')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

9) FilePathRuleTest::testRule with data set #22 (false, SimpleXMLElement Object (...), 'Изображений')
Failed asserting that true matches expected false.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

FAILURES!
Tests: 23, Assertions: 23, Failures: 9.
avatar richard67
richard67 - comment - 17 Mar 2021

applying this PR, against the unit tests (which are configured for staging, eg post 3.9.25 release) they fail:

@PhilETaylor All these are the fixes for the b/c introduced with 3.9.25, or not? What do your unit tests do on a 3.9.24?

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

And this people.... is why Unit tests ARE SO IMPORTANT!

avatar richard67
richard67 - comment - 17 Mar 2021

And this people.... is why Unit tests ARE SO IMPORTANT!

@PhilETaylor I've never said the opposite. But as you can see I am not an expert in test (yet).

As said, if you run your tests as they are on a 3.9.24 they should fail in the same way like now when you run them on my PR, right?

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

In order for Joomla 3.9.24 to pass the unit tests they need to be configured like this:

These all pass like this in Joomla 3.9.24

		    array(true, $xml, ''),
			array(true, $xml, '.images'),
			array(true, $xml, './images'),
			array(true, $xml, '.\images'),
			array(false, $xml, '../images'),
			array(false, $xml, '.../images'),
			array(true, $xml, 'c:\images'),
			array(true, $xml, '\\images'), // Means \images
			array(true, $xml, 'ftp://images'),
			array(true, $xml, 'http://images'),
			array(true, $xml, '/media'),
			array(true, $xml, '/administrator'),
			array(true, $xml, '/4711images'),
			array(true, $xml, '4711images'),
			array(true, $xml, '1'),
			array(true, $xml, '_'),
			array(true, $xml, '*'),
			array(true, $xml, '%'),
			array(true, $xml, '://foo'),
			array(true, $xml, '/4711i/images'),
			array(false, $xml, '../4711i/images'),
			array(true, $xml, 'Εικόνες'),
			array(true, $xml, 'Изображений'),

Then when I apply your PR to Joomla 3.9.24 I get failures:

There were 7 failures:

1) FilePathRuleTest::testRule with data set #2 (true, SimpleXMLElement Object (...), './images')
Failed asserting that false matches expected true.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

2) FilePathRuleTest::testRule with data set #3 (true, SimpleXMLElement Object (...), '.\images')
Failed asserting that false matches expected true.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

3) FilePathRuleTest::testRule with data set #7 (true, SimpleXMLElement Object (...), '\images')
Failed asserting that false matches expected true.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

4) FilePathRuleTest::testRule with data set #10 (true, SimpleXMLElement Object (...), '/media')
Failed asserting that false matches expected true.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

5) FilePathRuleTest::testRule with data set #11 (true, SimpleXMLElement Object (...), '/administrator')
Failed asserting that false matches expected true.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

6) FilePathRuleTest::testRule with data set #12 (true, SimpleXMLElement Object (...), '/4711images')
Failed asserting that false matches expected true.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

7) FilePathRuleTest::testRule with data set #19 (true, SimpleXMLElement Object (...), '/4711i/images')
Failed asserting that false matches expected true.

/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83

FAILURES!
Tests: 23, Assertions: 23, Failures: 7.

This tells me that your PR outputs something different than what Joomla 3.9.24 would have...

So... what you need to do is tell me which of these, because of SECURITY REASONS are no longer allowed, and because of SECURITY REASONS you are choosing to change these from true/false to true/false therefore creating a b/c break.

Because the above is the rules that pass on Joomla 3.9.24, they SHOULD pass on 3.9.25 and 3.9.26 else there has been a b.c break introduced somewhere.

So we know /media and /administator are a b/c break - there is another issue about that with a pending won't fix on it...

So the others that break are:

./images
.\images
\images
/4711images
/4711i/images

Which Im assuming is you making the security fix.... so it looks like the PR is acceptable, and the tests, once merged can be updated to take that into account.

avatar richard67
richard67 - comment - 17 Mar 2021

So the others that break are:

./images
.\images
\images
/4711images
/4711i/images

Which Im assuming is you making the security fix.... so it looks like the PR is acceptable, and the tests, once merged can be updated to take that into account.

@PhilETaylor Yes, these are the intended ones by the security fix.

Thanks for your help and analysis.

I agree with you that it makes sense to merge the tests first and then update this one here so one can see by the changes in the tests in this PR what this PR does.

avatar PhilETaylor PhilETaylor - test_item - 17 Mar 2021 - Tested successfully
avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

I have tested this item successfully on a753e1d

As per my comments above.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32718.

avatar PhilETaylor
PhilETaylor - comment - 17 Mar 2021

I agree with you that it makes sense to merge the tests first and then update this one here so one can see by the changes in the tests in this PR what this PR does.

remembering that the tests created today by me, are valid against the CURRENT staging, and not a released version (and so although they "pass" they "pass" validating the code added in 3.9.26 :-)...

So yes. 1) Merge the working unit tests 2) merge that into this pr 3) update this PR by toggling the true/false on the tests to make them pass again and push this PR again ...

avatar rdeutz
rdeutz - comment - 18 Mar 2021

We need to add changes the test for this PR, I would suggest for testing that testers apply first only the changes to the test and see if tests are failing.

Instructions for testing

  • get the latest staging
  • apply the changes for the unit tests in file tests/unit/suites/libraries/joomla/form/rule/FilePathRuleTest.phpfrom this PR.
  • execute the test -> should fail
  • apply the rest of this PR, i.e. the changes in file libraries/src/Form/Rule/FilePathRule.php.
  • execute the test -> should not fail

How the execute the tests?

  • go into the root of the checked out repository
  • do a "composer install"
  • do a "phpunit --filter FilePathRuleTest"

As of the writing of this comment changes for the test are not within the PR

avatar richard67
richard67 - comment - 18 Mar 2021

In some 2 hours I will have some time to update this PR by the new tests and adjust the tests. Please wait with the above testing until l’ve done that.

avatar joomla-cms-bot joomla-cms-bot - change - 18 Mar 2021
Category Libraries Libraries Unit Tests
avatar richard67
richard67 - comment - 18 Mar 2021

@PhilETaylor I've updated this PR and adjusted the tests.

But I've noticed a mistake in your test cases: They are testing /media and /administrator, but these don't fail due to the excluded core folder list, these fail because they start with a directory separator, i.e. absolute path, i.e. $path[0] is an empty string after the split of $value. So I'll add tests for media and administrator (without / at the beginning) and the exclude parameter to the form field as well. You should do the same in your PR #32724 .

Update: No, I was wrong with the above, that change should be made in J4 not now but later when merging up the security fix from staging after this PR has been merged. Sorry for confusion.

avatar richard67 richard67 - change - 18 Mar 2021
Labels Added: ?
avatar richard67
richard67 - comment - 18 Mar 2021

Drone failing now for unrelated reasons. Will restart from time to time until ok.

avatar richard67
richard67 - comment - 18 Mar 2021

Ready for testing.

@PhilETaylor Could you test again? Thanks in advance, and again thanks for good team work. Sorry, no need for retest, see comment below.

avatar richard67 richard67 - alter_testresult - 18 Mar 2021 - @PhilETaylor: Tested successfully
avatar richard67
richard67 - comment - 18 Mar 2021

I've restored @PhilETaylor 's test result because all changes after that test were only for the unit tests, not for the rule code.

avatar PhilETaylor
PhilETaylor - comment - 18 Mar 2021

All your work looks ok, my only note is that the Joomla 3 test suite's PHPUnit version is incompatible with PHP 8 and so this has not been tested (and actually fails in drone) in PHP 8.

Fatal error: Uncaught Error: Call to undefined function each() in /drone/src/libraries/vendor/phpunit/phpunit/src/Util/Getopt.php:38

That is code for "This PHPUnit version is incompatible with PHP 8" ;-)

avatar richard67
richard67 - comment - 18 Mar 2021

All your work looks ok, my only note is that the Joomla 3 test suite's PHPUnit version is incompatible with PHP 8 and so this has not been tested (and actually fails in drone) in PHP 8.

@PhilETaylor But I get it right that this is not related to my PR but also happens on current staging?

avatar PhilETaylor
PhilETaylor - comment - 18 Mar 2021

Correct. This is not your fault, I was nearly translating that error message into English so that you and others dont waste hours trying to fix the PHP 8 unit testing for Joomla 3. It will never work without upgrading PHPUnit and that then will break all the tests as the current PHPUnit version that is compatible with PHP8 is not compatible with the tests (and older PHP versions).

So, just ignore it.

Hint: I wasted hours trying to "fix" the unfixable PHPUnit error message :)

avatar alikon alikon - test_item - 18 Mar 2021 - Tested successfully
avatar alikon
alikon - comment - 18 Mar 2021

I have tested this item successfully on 4566d0e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32718.

avatar alikon alikon - change - 18 Mar 2021
Status Pending Ready to Commit
avatar alikon
alikon - comment - 18 Mar 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32718.

avatar rdeutz rdeutz - change - 18 Mar 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-03-18 18:45:29
Closed_By rdeutz
Labels Added: ? ?
Removed: ?
avatar rdeutz rdeutz - close - 18 Mar 2021
avatar rdeutz rdeutz - merge - 18 Mar 2021
avatar PhilETaylor
PhilETaylor - comment - 18 Mar 2021

Thank you @richard67 @alikon

avatar richard67
richard67 - comment - 18 Mar 2021

@PhilETaylor Thank you Phil.

avatar PhilETaylor
PhilETaylor - comment - 12 Apr 2021

I just fell foul of this a-z issue in Joomla 4 - does anyone know where @wilsonge is with merging up from staging the commits? Is it documented anywhere? is it still a one man effort?

avatar zero-24
zero-24 - comment - 12 Apr 2021

I just fell foul of this a-z issue in Joomla 4 - does anyone know where @wilsonge is with merging up from staging the commits?

hmm merging up usually happens after a release. So staging -> 3.10-dev -> 4.0-dev and it seems George merged the latest stuff up 7 days from today.
Following that logic would mean we would wait for 3.9.26 to be released, merged up to 3.10-dev and than merged up to 4.0-dev.

Is it documented anywhere?

What kind of docs are you looking for.

is it still a one man effort?

Merging up stuff is usually done by the release leads so all possible merge conflicts can be reviewed by that person too but sometimes other maintainers take that over too.

avatar PhilETaylor
PhilETaylor - comment - 12 Apr 2021

Well my concern is things might get missed.

However it seems the commits that I was concerned about were merged after the 3.9.25 was released, so from what you say they should not yet be in Joomla 4.

avatar zero-24
zero-24 - comment - 12 Apr 2021

However it seems the commits that I was concerned about were merged after the 3.9.25 was released, so from what you say they should not yet be in Joomla 4.

Now I'm confused what commits are you concerned about than? When its about this PR here it will be shipped tomorrow with 3.9.26 and has not been merged up yet?

avatar PhilETaylor
PhilETaylor - comment - 12 Apr 2021

Long story. Im having a bad day :)

Add a Comment

Login with GitHub to post a comment