Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow use factories and OM for creating objects with variadic arguments in constructor #24556

Conversation

adrian-martinez-interactiv4
Copy link
Contributor

Description (*)

Currently, if you have a class using a variadic argument in constructor, it is not possible to use generated factories or OM to instantiate it. Having this as example, there is no way to instantiate it using CustomObjectPoolFactory generated class:

/**
 * Demo class CustomObjectPool, accepting variable number of arguments, all of them of CustomObject type
 */
class CustomObjectPool
{
    /**
     * @var CustomObject[]
     */
    private $customObjects;

    /**
     * CustomObjectPool constructor.
     * @param CustomObject[] ...$customObjects
     */
    public function __construct(CustomObject ...$customObjects)
    {
        $this->customObjects = $customObjects;
    }
}

This is a test snippet, that fails with a type error exception:

<?php

use Magento\Framework\App\Area;

require __DIR__ . '/app/bootstrap.php';

/**
 * Demo class CustomObject
 */
class CustomObject
{
}

/**
 * Demo class CustomObjectPool, accepting variable number of arguments, all of them of CustomObject type
 */
class CustomObjectPool
{
    /**
     * @var CustomObject[]
     */
    private $customObjects;

    /**
     * CustomObjectPool constructor.
     * @param CustomObject[] ...$customObjects
     */
    public function __construct(CustomObject ...$customObjects)
    {
        $this->customObjects = $customObjects;
    }
}

/**
 * Class TestApp
 */
class TestApp extends \Magento\Framework\App\Http implements \Magento\Framework\AppInterface
{
    public function launch()
    {
        $this->_state->setAreaCode(Area::AREA_GLOBAL);
        $this->_objectManager->configure($this->_configLoader->load(Area::AREA_GLOBAL));

        // Used this way instead of constructor for convenience, not needed for ilustrative example
        $customObjectPoolFactory = $this->_objectManager->get(CustomObjectPoolFactory::class);
        // Line below throws type error exception
        $customObjectPool = $customObjectPoolFactory->create();
        // This line below also throws type error exception
        $customObjectPool = $customObjectPoolFactory->create(
            [
                'customObjects' => [
                    new CustomObject(),
                    new CustomObject(),
                ]
            ]
        );

        return $this->_response;
    }

    public function catchException(\Magento\Framework\App\Bootstrap $bootstrap, \Exception $exception)
    {
        var_dump($exception->getMessage());
        var_dump($exception->getTraceAsString());
        exit();
    }
}

$bootstrap = \Magento\Framework\App\Bootstrap::create(BP, $_SERVER);

/** @var \Magento\Framework\App\Http $app */
$app = $bootstrap->createApplication(TestApp::class);
$bootstrap->run($app);

With provided changes, variadic arguments are supported, including building without arguments:

$customObjectPool = $customObjectPoolFactory->create();

Or defining them as an array:

$customObjectPool = $customObjectPoolFactory->create(
            [
                'customObjects' => [
                    new CustomObject(),
                    new CustomObject(),
                ]
            ]
        );

Or defining a single one in a simple way:

$customObjectPool = $customObjectPoolFactory->create(
            [
                'customObjects' => new CustomObject()
            ]
        );

Or injecting them from di.xml config.

Fixed Issues (if relevant)

None AFAIK, improvement

Manual testing scenarios (*)

Use provided snippet.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Sep 11, 2019

Hi @adrian-martinez-interactiv4. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@magento-engcom-team magento-engcom-team added Component: Code Component: ObjectManager Release Line: 2.3 Partner: Interactiv4 Pull Request is created by partner Interactiv4 partners-contribution Pull Request is created by Magento Partner labels Sep 11, 2019
@buskamuza
Copy link
Contributor

Nice improvement. Did you have a chance to check if it would be possible to specify arguments in di.xml as well?

@adrian-martinez-interactiv4
Copy link
Contributor Author

@buskamuza I tested it here: https://github.com/magento/magento2/pull/24556/files#diff-3fe7b0651b076e3b1c628181bb34eca0R298, but having a second look at it, it may not work with real defined di.xml parameters, I'll review it.

@adrian-martinez-interactiv4
Copy link
Contributor Author

@buskamuza After reviewed, it seems it should work.

About Database Compare build failing, there are no build reports available.

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-5927 has been created to process this Pull Request
✳️ @ihor-sviziev, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@ihor-sviziev ihor-sviziev added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Award: complex labels Sep 23, 2019
@ihor-sviziev
Copy link
Contributor

@magento run Database Compare build

@engcom-Bravo
Copy link
Contributor

✔️ QA Passed

@engcom-Foxtrot engcom-Foxtrot self-assigned this Oct 14, 2019
magento-engcom-team pushed a commit that referenced this pull request Oct 22, 2019
@magento-engcom-team magento-engcom-team merged commit a2cbfa6 into magento:2.3-develop Oct 22, 2019
@m2-assistant
Copy link

m2-assistant bot commented Oct 22, 2019

Hi @adrian-martinez-interactiv4, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Oct 22, 2019
@adrian-martinez-interactiv4 adrian-martinez-interactiv4 deleted the FR23#FEATURE-VARIADIC-CONSTRUCTOR-ARGUMENTS branch October 24, 2019 14:31
adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Nov 12, 2020
…ts in constructor

Related with magento#24556.

That PR allowed to inject scalar values as variadic parameters, but do not work for objects injections. Object definitions don't get instantiated, and target class constructor receives an array of strings instead (with '_instance' key, that should have been transformed into real object)
adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Dec 9, 2020
…ts in constructor

Related with magento#24556.

That PR allowed to inject scalar values as variadic parameters, but do not work for objects injections. Object definitions don't get instantiated, and target class constructor receives an array of strings instead (with '_instance' key, that should have been transformed into real object)
adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Mar 15, 2021
…ts in constructor

Related with magento#24556.

That PR allowed to inject scalar values as variadic parameters, but do not work for objects injections. Object definitions don't get instantiated, and target class constructor receives an array of strings instead (with '_instance' key, that should have been transformed into real object)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants