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

Refactored Retrieval Of Entity ID To Make AbstractDataProvider Usable #21065

Merged
merged 3 commits into from
Mar 30, 2019
Merged

Refactored Retrieval Of Entity ID To Make AbstractDataProvider Usable #21065

merged 3 commits into from
Mar 30, 2019

Conversation

sprankhub
Copy link
Member

@sprankhub sprankhub commented Feb 8, 2019

Description (*)

When developing backend forms with the help of backend UI components, the Magento\Ui\DataProvider\AbstractDataProvider comes in handy. One can simply inherit from it, set one's own collection in the constructor and one could be done. However, in practice, it does not work this way, because Magento\Ui\Component\Form::getDataSourceData relies on the fact that the related entity has an id_field_name property. Since this is mostly not the case, one gets the following notice:

Notice: Undefined index: id_field_name in /var/www/magento2/vendor/magento/module-ui/Component/Form.php on line 77

Since one needs to define the primary field name in the data provider XML config anyway, it should be much more robust to get the primary field name from there. It is already used in the filter anyway.

Fixed Issues (if relevant)

  1. n/a

Manual testing scenarios (*)

  1. Create a custom backend UI component form.
  2. Implement a data provider, which inherits from Magento\Ui\DataProvider\AbstractDataProvider and simply sets a custom collection in the constructor, e.g. a simple order collection of type Magento\Sales\Model\ResourceModel\Order\Collection.
  3. Open the UI component in the backend in developer mode.
  4. See the notice quoted above.

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 8, 2019

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @sprankhub. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

Copy link
Contributor

@swnsma swnsma left a comment

Choose a reason for hiding this comment

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

Hi @sprankhub,
Could I ask you to cover current method with unit tests?

@sprankhub
Copy link
Member Author

@swnsma I will try :-)

@sprankhub
Copy link
Member Author

@swnsma I added a unit test now, which fails without this PR and passes with it.

@swnsma
Copy link
Contributor

swnsma commented Feb 21, 2019

Hi @sprankhub,
Thanks!

Could you please check your last commit? It seems to be authored by git user(email) which not related to your current account. You can change commit author with force-push.

@sprankhub
Copy link
Member Author

No, it was just not created via github.com, but via my local system. I am currently unable to signoff my last commit :-(

Is this a blocker? I see other unverified commits in the history, so it does not seem to be an issue?

@nilesh2jcommerce
Copy link

nilesh2jcommerce commented Feb 21, 2019 via email

@ajay2jcommerce
Copy link

ajay2jcommerce commented Feb 21, 2019 via email

@priti2jcommerce
Copy link
Contributor

priti2jcommerce commented Feb 21, 2019 via email

@dipti2jcommerce
Copy link

dipti2jcommerce commented Feb 21, 2019 via email

@prakash2jcommerce
Copy link

prakash2jcommerce commented Feb 21, 2019 via email

@nainesh2jcommerce
Copy link
Contributor

nainesh2jcommerce commented Feb 21, 2019 via email

@nilesh2jcommerce
Copy link

nilesh2jcommerce commented Feb 21, 2019 via email

@prakash2jcommerce
Copy link

prakash2jcommerce commented Feb 21, 2019 via email

@prakash2jcommerce
Copy link

prakash2jcommerce commented Feb 21, 2019 via email

@priti2jcommerce
Copy link
Contributor

priti2jcommerce commented Feb 21, 2019 via email

@nainesh2jcommerce
Copy link
Contributor

nainesh2jcommerce commented Feb 21, 2019 via email

@prakash2jcommerce
Copy link

prakash2jcommerce commented Feb 21, 2019 via email

@swnsma
Copy link
Contributor

swnsma commented Feb 21, 2019

Hi @sprankhub,
Thank you for your changes!
It looks like OK for now, is not it?

@sprankhub
Copy link
Member Author

No, the commit is still not verified.

@sivaschenko
Copy link
Member

@swnsma @sprankhub licence/cla check is green. Comment #21065 (comment) seems to be outdated

@swnsma
Copy link
Contributor

swnsma commented Feb 21, 2019

@sprankhub
Travis will handle this. Lets just give it time.

@swnsma swnsma self-requested a review February 22, 2019 08:44
@magento-engcom-team
Copy link
Contributor

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

@m2-assistant
Copy link

m2-assistant bot commented Mar 30, 2019

Hi @sprankhub, 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.2 milestone Mar 30, 2019
@sprankhub sprankhub deleted the sprankhub-patch-1 branch August 30, 2019 08:07
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.