FIX HTTPBulkToolsResponse.addSuccessRecords() fn

[pschiffe]

Description

This function was trying to merge 2 arrays with PHP fn array_push() which was failing if the second parameter was array as well. This fix replaces array_push() with array_merge() fn.

Manual testing steps

See the linked issue.

Issues

• addSuccessRecords() and addFailedRecords() return invalid JSON #187

Pull request checklist

• The target branch is correct
  • See picking the right version
• All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• The commit messages follow our commit message guidelines
• The PR follows our contribution guidelines
• Code changes follow our coding conventions
• This change is covered with tests (or tests aren't necessary for this change)
• Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• CI is green

[GuySartorelli]

There is not enough information in the linked issue for me to quickly reproduce the bug nor test this PR. Can you please add some detailed reproduction/test steps?

[pschiffe]

Hm, I don't really have any special config I think:

class A extends DataObject {

    private static $many_many = array(
        'B' => 'B',
    );

    public function getCMSFields() {
        $fields = parent::getCMSFields();

        if ($this->exists()) {
            $bulkManager = new BulkManager();
            $bulkManager->removeBulkAction(EditHandler::class);
            $bulkManager->removeBulkAction(DeleteHandler::class);

            $config = $fields->dataFieldByName('B')->getConfig();
            $config->addComponent($bulkManager);
            $config->getComponentByType(GridFieldAddExistingAutocompleter::class)
                ->setSearchList(B::get());
            $fields->dataFieldByName('B')->setConfig($config);
        }

        return $fields;
    }

Does this help?

[GuySartorelli]

That might help, but what I really need are steps to follow.
From a fresh installation of Silverstripe CMS with this module, what steps do I need to take to trigger the original error?

[pschiffe]

In the admin, I see this error when I try to unlink one or more instances of B class from A class, via the BulkManager.

[GuySartorelli]

I had to set up the following in order to follow those steps:

expand to see code

This is similar to what you had, but with missing use statements and closing brace

namespace App;

use Colymba\BulkManager\BulkAction\DeleteHandler;
use Colymba\BulkManager\BulkAction\EditHandler;
use Colymba\BulkManager\BulkManager;
use SilverStripe\Forms\GridField\GridFieldAddExistingAutocompleter;
use SilverStripe\ORM\DataObject;

class A extends DataObject
{
    private static $many_many = [
        'B' => B::class,
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();

        if ($this->isInDB()) {
            $bulkManager = new BulkManager();
            $bulkManager->removeBulkAction(EditHandler::class);
            $bulkManager->removeBulkAction(DeleteHandler::class);

            $config = $fields->dataFieldByName('B')->getConfig();
            $config->addComponent($bulkManager);
            $config->getComponentByType(GridFieldAddExistingAutocompleter::class)
                ->setSearchList(B::get());
            $fields->dataFieldByName('B')->setConfig($config);
        }

        return $fields;
    }
}

I also needed a B class that you reference but didn't include, and a model admin for these. Thankfully in this case winging it worked out, but if this hadn't worked I would have had to ask for more information again. It's always better for you to include what you think is too much information, rather than potentially not including enough ;p

namespace App;

use SilverStripe\ORM\DataObject;

class B extends DataObject
{

}

namespace App;

use SilverStripe\Admin\ModelAdmin;

class AlphabetAdmin extends ModelAdmin
{
    private static $url_segment = 'alphabet';
    private static $menu_title = 'ALPHABET';

    private static $managed_models = [
        A::class,
        B::class,
    ];
}

With that I can both reproduce the original error and see that the PR resolves it.

[GuySartorelli]

Looks good to me. Works well locally.
It's a little concerning that this is a bug in the CMS 4 version of the module and hasn't bothered anyone enough to fix until now. 😅

We really need to add tests to this module. If you'd like to tackle that I'd definitely support you in it.

In the meantime, thanks a bunch for fixing this!

[GuySartorelli]

Failed JS CI job is unrelated to PR changes.

[pschiffe]

Thank you @GuySartorelli for your time working on this as well for the feedback on my PR, I really appreciate it.
I'm not really a developer anymore, just trying to update one small project I did long time ago :-)