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

Fixed plugin generation issues #612

Merged
merged 6 commits into from
Jul 12, 2021

Conversation

bohdan-harniuk
Copy link
Collaborator

Description (*)
I've prettified the plugin generation feature.

What was done:

  1. fixed empty line after a class opening tag and before plugin method
  2. fixed useless FQCN for subject method argument + in the PHP DOC
  3. fixed wrong argument name in the PHP DOC for after plugin's original method argument
  4. additionally fixed wrong null type in the PHP DOC for original method arguments and return type (not mentioned in issue The content of the generated plugin class has problems #605, for example: ?int or ?SomeType that actually should be int|null and SomeType|null)
  5. additionally fixed wrong returning for void plugin methods
  6. additionally added an improvement to render strict type for plugin method return

Screenshot 2021-06-25 at 12 51 15

Screenshot 2021-06-25 at 12 51 28

Screenshot 2021-06-25 at 12 53 18

Fixed Issues (if relevant)

  1. Fixes The content of the generated plugin class has problems #605

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 integration/functional tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@bohdan-harniuk bohdan-harniuk force-pushed the fix-plugin-generation branch from d61089e to 6daa446 Compare June 25, 2021 13:34
Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for these fixes.
Could you please review my failed test cases?

Case 1

    public function catchException(Bootstrap $bootstrap, \Exception $exception)
    {
        return false;
    }

As result, we have \Exception as param type, which is already added to use list. It is reproducible for all plugin types.

    /**
     * @param UserConfig $subject
     * @param Bootstrap $bootstrap
     * @param Exception $exception
     * @return array
     */
    public function beforeCatchException(UserConfig $subject, Bootstrap $bootstrap, \Exception $exception): array
    {
        // TODO: Implement plugin method.
        return [$bootstrap, $exception];
    }

Case 2

After and Around generations result in missing strict returning type. I believe we can also add the return type if is known from the method's docs.

    /**
     * @param BackendApp $subject
     * @param callable $proceed
     */
    public function aroundGetCookiePath(BackendApp $subject, callable $proceed):
    {
        // TODO: Implement plugin method.
        return $proceed();
    }

Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing the previous issues. Please find out the following test case that I could find out.

Case 1

For After/Around plugins

    /**
     * @return \Magento\Framework\DB\Select
     */
    public function getSelectCountSql()
    {
        /* @var $countSelect \Magento\Framework\DB\Select */
        $countSelect = parent::getSelectCountSql();
        $countSelect->resetJoinLeft();
        return $countSelect;
    }

Results in not adding the Select class to use section. The same happens with imported method's params.

    /**
     * @param Collection $subject
     * @param Select $result
     * @return Select
     */
    public function afterGetSelectCountSql(Collection $subject, Select $result): Select
    {
        // TODO: Implement plugin method.
        return $result;
    }

@bohdan-harniuk
Copy link
Collaborator Author

Hello, @eduard13

I've successfully reproduced your last case:
Screenshot 2021-07-05 at 13 55 16

I've fixed that:
Screenshot 2021-07-05 at 15 46 59

Could you please check if everything is fine now? I created a test class for this, I could miss something.

Thanks, Bohdan

Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the great effort on this one 👏

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.

The content of the generated plugin class has problems
3 participants