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

Execute collection tasks only when a valid collection is passed to th… #13574

Conversation

mgarciaebo
Copy link

@mgarciaebo mgarciaebo commented Feb 8, 2018

…e data source of a UI Component form. This prevents ui forms not attached to collections to crash on filtering

Description

There should be some check in order to perform collection actions only if the data source of UI Componet forms contents and actual valid collection, and avoiding them if not. This keeps model-collections forms working as usual, and avoids crashes on stand-alone forms.

Fixed Issues (if relevant)

  1. UI Component forms not related with any model throw fatal error #13573: UI Component forms not related with any model throw fatal error

Manual testing scenarios

  1. Starting from a vanilla Magento, download, install and enable your own sample Form UI Component, following the instructions given at: https://github.com/magento/magento2-samples/tree/master/sample-module-form-uicomponent
  2. Go to the URL provided by the extension, which is [magento2-admin-url]/sampleform
  3. You should now get the expected form:
    sample_form

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 on Travis CI are green)

…e data source of a UI Component form. This prevents ui forms not attached to collections to crash on filtering
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 8, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@rogyar rogyar self-assigned this Mar 16, 2018
->addFilter($filter);
$dataProvider = $this->getContext()->getDataProvider();
if (isset($dataProvider->collection)) {
$this->getContext()->getDataProvider()
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we don't need invoking $this->getContext()->getDataProvider() once again since we have added $dataProvider variable

@rogyar
Copy link
Contributor

rogyar commented Mar 16, 2018

@mgarciaavz thank you for your contribution.
Please, consider fixing the failing tests and the issues described above.

@mgarciaebo
Copy link
Author

Thank you for your comment @rogyar, I already implemented as you mentioned and fixed the failing tests (still waiting for Travis' results 🤞)

@mgarciaebo mgarciaebo added the Event: distributed-cd Distributed Contribution Day label Mar 24, 2018
@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-1092 has been created to process this Pull Request

@rogyar
Copy link
Contributor

rogyar commented Aug 9, 2018

Hi @mgarciaavz. Unfortunately, the provided changes bring failing performance tests. The form UI component changes significantly slow down the rendering time:

Server Side EE Admin Customer Management:
Edit Customer 886.00ms +135.6% (+510ms) DEGRADATION
Server Side EE Admin Edit Product:
Edit Simple Product 560.00ms +34.3% (+143ms) DEGRADATION
Edit Simple Product Save 1043.00ms +16.0% (+144ms) DEGRADATION
Edit Configurable Product 730.00ms +21.9% (+131ms) DEGRADATION

So, we need to adjust the approach in order to avoid performance issues. Thank you.

@rogyar
Copy link
Contributor

rogyar commented Aug 22, 2018

I'm closing this PR due to inactivity. Please, feel free to reopen it if you decide to proceed. Thank you

@rogyar rogyar closed this Aug 22, 2018
@mgarciaebo
Copy link
Author

mgarciaebo commented Oct 8, 2018

Reopening this PR. I'll try a different approach.

@mgarciaebo mgarciaebo reopened this Oct 8, 2018
… an UI form uses a collection and avoid unexpected collection filtering errors
@sivaschenko
Copy link
Member

Hi @mgarciaavz , thanks for collaboration! If you'd like to continue working on this contribution, please port this pull request to 2.3-develop branch as this improvement should first be introduced to 2.3: https://devdocs.magento.com/guides/v2.2/contributor-guide/contributing.html#rules

@sidolov
Copy link
Contributor

sidolov commented Dec 6, 2018

Hi @mgarciaavz , will you continue with the PR?

@sidolov
Copy link
Contributor

sidolov commented Dec 9, 2018

Hi @mgarciaavz , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Dec 9, 2018
@mgarciaebo mgarciaebo deleted the hotfix/ui-form-no-collection-fatal branch October 31, 2019 09:44
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.

7 participants