Not tested, but in principle. Tests pass so that's a good sign.

Tested on latest 3.2.3. Applied patch and nothing broke! Also installed some custom extensions I've written and can confirm that those did not break either.

We should bring in the Symfony Yaml component as well, to round out the functionality. Seems weird to have the code to use it but not the required dependency.

Consider it done then. symfony/yaml 2.4.1 is added (butchered up to not include the tests and other stuff that comes with a normal Composer install, we can deal with that when we move to a proper Composer installation in the CMS).

Agree. Let's worry about Symfony in 3.4 (being hopeful).

You need to delete the registry files in com_admin's script

com_admin is taken care of now.

Pumped this into my site and navigated between a few changes and looks good to me :) can't see anything obvious breaking. Obviously haven't tried anything too serious though like saving data from other components etc.

Looks good to me. Has multiple successful tests. Merging to dev.

This merge has broken installation.
1: we get a Notice:
! ) Notice: Undefined index: site_metadesc in /Applications/MAMP/htdocs/testnew/33dev/installation/view/summary/tmpl/default.php on line 86
2. Then we are stuck after creating the configuration.php file and can't go further.

Reverting this PR solves the issue

I get a good install on PHP 5.4.24 with out without a site description. JM, you'll need to provide your full system spec's and what installation options you used.

I got the same on my homeserver. empty site_metadesc and empty "sample_data" triggered that notice and failed the installation process.
On my xampp server it's even worse because I have an empty database password. It triggers a notice for undefined "db_pass" (or so) and breaks step 2.

This fixes it:
Bakual@joomla:3.3-dev...FixInstallation
But breaks Travis:

There was 1 failure:
1) JRegistryTest::testMerge
$a value should override blank $b value
Failed asserting that null matches expected 'value2'.
/home/travis/build/Bakual/joomla-cms/tests/unit/suites/libraries/joomla/registry/JRegistryTest.php:499

I have no clue how to really fix the issue. But there for sure is one.

Try replacing bindData with this:

protected function bindData($parent, $data, $recursive = true)
{
    // Ensure the input data is an array.
    if (is_object($data))
    {
        $data = get_object_vars($data);
    }
    else
    {
        $data = (array) $data;
    }

    foreach ($data as $k => $v)
    {
        // Modification from joomla/registry package - Using CMS JArrayHelper versus importing joomla/utilities package
        if ($recursive && ((is_array($v) && \JArrayHelper::isAssociative($v)) || is_object($v)))
        {
            if (!isset($parent->$k))
            {
                $parent->$k = new \stdClass;
            }
            $this->bindData($parent->$k, $v);
        }
        else
        {
            $parent->$k = $v;
        }
    }
}

So you've just removed that line @Bakual talked about and moved the definition of recursive right? I'm pretty sure that won't make a difference because I tried applying his fix and set the default value of true for the merge function - so recursive was always true and it didn't fix the failing test

Bakual@joomla:3.3-dev...FixInstallationDon
Installation works (as expected).
Waiting for Travis

Travis failed: https://travis-ci.org/Bakual/joomla-cms/jobs/22106291

I have not been able to consistently reproduce the installation breaking. It currently works for me whereas it didn't 22 hours ago. This is on an Ubuntu box running Apache2 with PHP 5.3.10 tested with and without a password being used for the database user.

I think this fixes the issues https://github.com/wilsonge/joomla-cms/compare/joomla:3.3-dev...FixInstallation?expand=1 but I have a feeling after this that merging recursively may not work with the same issue as before that it will merge the subclasses incorrectly. But as that is a new param - this keeps the old CMS behaviour

@wilsonge no, read the original code again and compare it to what I posted. I didn't "just move the definition of recursive". In the current iteration, the recursive check is dependent upon the value not being an array, due to a misplaced parenthesis. You should only ever enter the recursive section if $recursive === true. The original code first checked if it was an associative array, then did an || check on is_object($v) && $recursive, so the $recursive check was never run correctly.

Both original @bakual and @dongilbert patches above do solve the installation issue here.
Where do we stand now?

Testing my patch - because so the unit tests still pass

The unit tests aren't the end all be all on code acceptance. Proper code
review and testing is still required.

On Wednesday, April 2, 2014, George Wilson notifications@github.com wrote:

Testing my patch - because so the unit tests still pass

Reply to this email directly or view it on GitHub#3374 (comment)
.

@infograf768 My code and the one from Don both don't pass the unit tests for me (but installation works with both). So either the unit tests are wrong, or something with the Registry package is still wrong.
I haven't tested Georges yet, so can't comment there. But will try to test later.

So the issue is in the merge function apparently if the value of a cell is blank or an empty string then the existing value takes priority. Now when the recursive method was moved into the merge function, the code that dealt with the merging was also moved into bindData

However basically the issue is if we move that piece code back into merge rather than being in bindData then everything works as expected

I think George is right. However I'm not sure if we need to move it back to the merge (potentially breaking the recursive part). I think it should be possible to do in the bindData method.

I'm to tired at the moment to wrap my mind around it anymore, but I think we're close

Guys, can I have a little more information to go on. I always get nervous when we are trying to fix intermittent errors without establishing the root cause.

If you run through an install of the CMS, there's some PHP notices to be found (though install completes successfully, I do have them in my own error_log). Basically, it revolves around empty values not being stored in an options array passed through the AJAX requests and apparently the cause was this PR.

[01-Apr-2014 17:24:44 America/Chicago] PHP Notice:  Undefined index: site_metadesc in /Applications/MAMP/htdocs/joomla-cms/installation/view/summary/tmpl/default.php on line 86
[01-Apr-2014 17:24:44 America/Chicago] PHP Stack trace:
[01-Apr-2014 17:24:44 America/Chicago] PHP   1. {main}() /Applications/MAMP/htdocs/joomla-cms/installation/index.php:0
[01-Apr-2014 17:24:44 America/Chicago] PHP   2. JApplicationCms->execute() /Applications/MAMP/htdocs/joomla-cms/installation/index.php:27
[01-Apr-2014 17:24:44 America/Chicago] PHP   3. InstallationApplicationWeb->doExecute() /Applications/MAMP/htdocs/joomla-cms/libraries/cms/application/cms.php:255
[01-Apr-2014 17:24:44 America/Chicago] PHP   4. InstallationApplicationWeb->dispatch() /Applications/MAMP/htdocs/joomla-cms/installation/application/web.php:222
[01-Apr-2014 17:24:44 America/Chicago] PHP   5. InstallationControllerDefault->execute() /Applications/MAMP/htdocs/joomla-cms/installation/application/web.php:183
[01-Apr-2014 17:24:44 America/Chicago] PHP   6. InstallationViewSummaryHtml->render() /Applications/MAMP/htdocs/joomla-cms/installation/controller/default.php:117
[01-Apr-2014 17:24:44 America/Chicago] PHP   7. InstallationViewDefault->render() /Applications/MAMP/htdocs/joomla-cms/installation/view/summary/html.php:58
[01-Apr-2014 17:24:44 America/Chicago] PHP   8. JViewHtml->render() /Applications/MAMP/htdocs/joomla-cms/installation/view/default.php:48
[01-Apr-2014 17:24:44 America/Chicago] PHP   9. include() /Applications/MAMP/htdocs/joomla-cms/libraries/joomla/view/html.php:150

Personally, I doubt Joomla\Registry is the cause. The options array in question is never used as a JRegistry object but a PHP array. So I don't know what this PR did to start causing those notices or other sporadic errors that @infograf768 @betweenbrain or @Bakual have had, but the blame is being placed here.

It is the cause. The issue is that this line: https://github.com/joomla/joomla-cms/pull/3374/files#diff-7aa98967e872a113d4278d4b89def2f3R490 was moved from the merge function here: https://github.com/joomla/joomla-cms/pull/3374/files#diff-7aa98967e872a113d4278d4b89def2f3L316

Unfortunately the bindData function has a lot more uses than just merge. Which means if you construct a JRegistry object and have a blank string or null variable on construction it is longer put in the JRegistry object.

Using this gist as a reference piece - https://gist.github.com/mbabker/9944151

That gist is a dump of data during the first AJAX request in the install app. InstallationModelSetup::validate() calls JForm::filter() and passes the data array with the user input. In JForm, it holds data in a JRegistry object, and it is there where the empty keys are getting stripped. So, I was wrong about it not being a JRegistry issue.

For what it's worth, the empty key is there in 3.2.

Try this one - mbabker@joomla:3.3-dev...installFix

Takes @dongilbert code and fixes up the merge function too. Passed tests locally and lets you install.

So that's the code I did but with @dongilbert fix but my issue is that if you do a recursive merge only the higher level merge stops the null fields - but all the children with null values do get merged. Which seems pretty inconsistent

Not quite. When the change was made in the Framework, the line checking if the value was a null or empty string was changed from a && comparison to a ||. This is why your fix still has issues, you're using that latter incorrect logic.

Ahhh ok. Nice spot. I still have an issue with the children null values vs. parent null values seems pretty inconsistent

We can address that separately, it's not related to this specific issue.

True but as soon as we make a CMS release then that becomes expected behaviour and i guess it becomes a b/c issue to address it?

It's broken in the Framework too and honestly probably goes back even to the Platform code. Unless you can prove there's a break in logic. Once you're in the bindData() method, there's nothing sending you back to merge if you came in through that method to do the cleanup.

It's a new feature that went into the framework early on being able to merge the children - I ended up going back to through the code earlier when I was trying to figure out the bug. As far as I can tell there's no break in logic from before. It just means the feature implementation is kinda inconsistent. If you guys are happy with that tho thats fine :)

Wouldn't say happy with it but if the API's consistent with the previous implementation then we're at least on the right track. Working out the merge issue for child items is something that's going to take more thinking.

It does mean this code is not b/c with the framework code though. Because the framework current version does do that merge on child items

It's a bug to be fixed upstream.

joomla/joomla-framework@ae19714 PR that introduced this. I think the rest is ok