Skip to content

Only load attributes from attribute set of given entity type - #39794

Open
Maikel-Koek wants to merge 2 commits into
magento:2.4-developfrom
Maikel-Koek:2.4-develop
Open

Maikel-Koek wants to merge 2 commits into
magento:2.4-developfrom
Maikel-Koek:2.4-develop

Conversation

@Maikel-Koek

@Maikel-Koek Maikel-Koek commented Apr 9, 2025 •

Copy link
Copy Markdown
Contributor

Description

The eav_entity_attribute table can contain attributes of different entity types (from third parties), next to the catalog_product entity type attributes. These attributes can possibly have the same attribute set IDs as the catalog_product entity attribute set IDs.
This can cause issues when loading the attributes by attribute set for a product.

For example: when switching a product's attribute set to a different attribute set, the following snippet will try to delete the data of the product of the old attribute set:
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Catalog/Model/ResourceModel/Product.php#L344-L355

This will result in the following error:

Call to a member function getBackendTable() on bool

On: https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Catalog/Model/ResourceModel/Product.php#L373:

Manual testing scenarios

  1. Create a custom entity type, or install a third party extension which adds custom entity types (like https://mirasvit.com/magento-2-extensions/blog.html, which adds blog_post and blog_category entity types), and make sure you have a product attribute set ID with the same attribute set ID of the custom entity type.
  2. Try to switch the attribute set of an existing product, which currently has a product attribute set ID which is the same as one of the custom entity type IDs.
  3. When trying to save the product, the following error will appear:
    Call to a member function getBackendTable() on bool

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)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Only load attributes from attribute set of given entity type #39815: Only load attributes from attribute set of given entity type

@m2-assistant

m2-assistant Bot commented Apr 9, 2025

Copy link
Copy Markdown

Hi @Maikel-Koek. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@Maikel-Koek

Copy link
Copy Markdown
Contributor Author

@magento run all tests

@Maikel-Koek

Copy link
Copy Markdown
Contributor Author

@magento It looks like tests are failing that have nothing to do with my changes.

@hostep

hostep commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

The unit test failures do seem related to changes introduced in this PR:

There was 1 error:

  1. Magento\Eav\Test\Unit\Model\AttributeManagementTest::testGetAttributes
    Error: Call to a member function load() on null

/var/www/html/app/code/Magento/Eav/Model/AttributeManagement.php:187
/var/www/html/app/code/Magento/Eav/Test/Unit/Model/AttributeManagementTest.php:429

So maybe the unit tests found a bug in this change, or they should be updated. I'll leave that up to you to figure out.

@engcom-Hotel

Copy link
Copy Markdown
Contributor

@magento create issue

@engcom-Hotel engcom-Hotel added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Apr 15, 2025
@engcom-Hotel engcom-Hotel added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Apr 15, 2025
@github-project-automation github-project-automation Bot moved this to Pending Review in Pull Requests Dashboard Apr 15, 2025
@engcom-Hotel

Copy link
Copy Markdown
Contributor

Hello @Maikel-Koek,

Just wanted to followup with you on the issue reproduction, please refer to this #39815 (comment). We are unable to reproduce the issue mentioned here.

Please let us know if we missed anything, meanwhile we are moving this PR On hold.

Thanks

@lbajsarowicz

Copy link
Copy Markdown
Contributor

@Maikel-Koek I still see no entity-type filter in AttributeManagement::getAttributes(). Please respond to @engcom-Hotel with exact reproduction steps and an eav_entity_attribute fixture showing the cross-type rows and failing product attribute-set switch; eav_attribute_set.attribute_set_id is globally unique. Please also fix @hostep's reported failure: update testGetAttributes() to expect the fluent addFieldToFilter() call and add regression coverage for excluding cross-type rows. If you no longer have time, I can take this over with credit to you.

@Maikel-Koek

Copy link
Copy Markdown
Contributor Author

@lbajsarowicz I believe I created a workaround for this at the time, which is a while ago. You may investigate this further; I currently do not have the time for this.

@lbajsarowicz

Copy link
Copy Markdown
Contributor

@Maikel-Koek thanks for handing it over. Continued in #41724, which keeps your commit. On 2.4-develop the crash reproduces once an attribute of another entity type is linked to a product attribute set in eav_entity_attribute: moving a product out of that set fails with Call to a member function getBackendTable() on false. #41724 filters on the attribute's own entity_type_id and adds the unit test update plus an integration regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: on hold Progress: pending review Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it

Projects

Development

Successfully merging this pull request may close these issues.

[Issue] Only load attributes from attribute set of given entity type

4 participants