-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
GUI to Edit EAV Attributes & Sets - Base Code + Category #2317
base: main
Are you sure you want to change the base?
Conversation
The basic code structure is that we have generic blocks in Mage_Eav that make grids and edit forms for any EAV attribute. To determine which one we are editing, we read from the registry. Then we also have an abstract controllers that take care of the grid/edit/save/etc actions. For each EAV attribute we want to allow the user to edit, we just add the menu item and extend from the abstract controllers. |
Also for discussion, what is the reason someone would edit any of the Sales (Orders, Invoice, Creditmemo, Shipment) EAV attributes, or attribute sets? Has anyone ever done anything with these EAV entities in custom code? Would we want to show extra attributes in admin create order screens? |
I did that a lot of times in the past 12 years :-D |
I love this PR but I think there's a problem, orders/invoice etc have flat tables with static attributes (meaning columns in the flat table), not in the EAV. If I add an order attribute I want to have it on the flat table (maybe even on the _grid table) but not in the EAV... I also don't know how much sense would the attribute_set feature have, since they're _flat tables... same thing for the "scope". Same thing for the customer attribute sets, I'm not sure to force user to choose an attribute set for every customer group.. I'd leave that part for a future implementation (and that's why I didn't include it in my original PRs) |
2069699
to
52f77ee
Compare
I add columns to the flat table quite often as well, I just wasn't thinking of it as using EAV attributes, but I guess it is technically? Did Magento used to use the EAV table for orders before they decided on the flat tables for performance? Anyway, so should we just remove all Sales > Attributes menu for now? It's as easy as dropping that one commit.
I was thinking similar @kiatng's comment in the other PR:
Where customer groups would by default use the default attribute set, so the user would only need to change it if they wanted to. But more than that, the custom attributes do not even show up anywhere in the Customer > Edit screen yet, we need the |
52f77ee
to
95e498d
Compare
All sales entities do not use EAV. There is no trace of it except in the table
magento-lts/app/code/core/Mage/Sales/Model/Resource/Collection/Abstract.php Lines 57 to 68 in 042d4e5
So, only the catalog and customer module use EAV. Recently, I have implemented a custom EAV module. So, for me, I can see a need to abstract common functions to the EAV module instead of extending to the catalog module, and as pointed out by @fballiano, create unnecessary dependency. I use attribute sets to organize the attributes for different customer groups. The project has over 200 customer attributes, each customer group having very different set of attributes. My concern is that changes to EAV could make it very difficult to merge OM to existing projects with custom EAV. |
So if the only EAV entities are actually customer, customer_address, catalog and category maybe my approach wasn't so bad after all :-D but... somebody could have custom EAV entities and it would be awesome to manage their attributes with a GUI anyway... so maybe we could remove the 4 non-EAV entities from this PR and postpone it after merging this one? |
So let's definitely just remove the catalog EAV grids. Technically you could add attributes & sets for orders, invoices, etc, but with no way to retrieve those values in the collections it's pointless and confusing.
The PR as it now should definitely not conflict with anything since it's all new code, but I see your point if we hook up a customer group -> customer attribute set relation. Still, I think it could be worth it maybe just waiting to merge this PR until the next major version. How did you correlate attribute sets with customer groups? My first thought is to modify the
To add something like If we can do it in a way that is maybe compatible with what you've done, then that's at least one site that wouldn't break. Okay so next question, is about attribute sets on categories. I can't see a need for this, I just included it in the PR since it was only a few lines to do so. How should the menu entries look like? A few options:
Option 1 is like I have it now in this PR, just with the Category Attribute Sets option removed. Option 2 is like @fballiano had it but with the word "Product" added to the first two options. Option 3 is a bit weird because I don't think there's other cases where a clickable menu item has children. |
Yeah, your original PRs were definitely on the money here. I just thought it could be abstracted and then I added all the EAV entities since it only requires adding the controller & adminhtml.xml entries. I definitely would consider now to make my own EAV entities though, since the GUI boilerplate is taken care of. I reverted the catalog grids in a commit, I am not sure what you mean by the second part (postpone what?) P.S. I will squash and force push some of these commits before this PR is merged (if it is merged). I just didn't want to keep force-pushing in case anyone has cloned this branch. |
I category_attribute_set should be tied to the store, since you may want to have different attribute sets for categories in different websites (but I'd use store, not store_views, since the store is the root category).
I wouldn't change the products attribute admin path that everybody has now in production, since we'd need an update script to fix the ACL. So I think I'd go for option 2. |
I mean that this PR shouldn't do anything about order/quote/invoice etc |
+1, that makes sense to me. Is it too much to add in to this PR the code to associate category attribute sets to stores, and customer/customer address attribute sets to customer groups? Should that be a follow up PR?
If we allow the Category Attribute Sets option, then I am leaning towards this ideally:
To fix the BC break, I made PR #2321 that should allow us to change the ACL & menu structure without changing the ACL path. |
I'd do it in a separate PR cause I think it's not everybody's use case and one may want to add/edit attributes and write custom code to handle them on the frontend side
To me it's not super necessary to change the path and introduce that other PR to handle that.
it's never extremely comfortable to navigate many sublevels plus there would be only 4 items in the "attributes" one, not a very messy situation ;-) |
Yes, sounds good.
Okay, I agree with that. The other reason for PR #2321 was to allow setting a custom ACL path to |
I pushed the menu changes. A few other TODOs: Block the user from editing/deleting "system" attributesShould we do this? It seems like we are not blocked from editing system product attributes. I suppose an admin could probably do some damage by editing those items, but I do not know what will happen. Review the SQL commands at the end of this comment: #2264 (comment)UPDATE eav_attribute SET frontend_input = 'boolean' WHERE source_model = 'eav/entity_attribute_source_boolean' AND entity_type_id = 3;
DELETE eag.* FROM eav_attribute_set eas LEFT JOIN eav_attribute_group eag ON eas.attribute_set_id = eag.attribute_set_id WHERE eas.entity_type_id = 3 AND eag.attribute_group_name = 'General';
UPDATE eav_attribute_set eas LEFT JOIN eav_attribute_group eag ON eas.attribute_set_id = eag.attribute_set_id SET eag.default_id = 1 WHERE eas.entity_type_id = 3 AND eag.attribute_group_name = 'General Information'; I think it's a bug that we had Also, we can clean up some the unused attribute group name "General" for the default product category attribute set. I appreciate the feedback and of course your original initiative on this @fballiano, I do not want to let this PR stagnate so I'm trying to work through all the issues. :) |
it's possible to edit system attributes but not delete them, I think we should have the same policy here. Although... let's take "media_gallery" attribute as an example... I can't imagine what's happening if you modify the "scope" of this attribute but it's been like this since the beginning of magento so... |
About the SQL query review, I couldn't test it but maybe it should be a PR on its own? |
@fballiano It shouldn't be possible right now to delete system attributes because of this line: magento-lts/app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Edit.php Lines 51 to 55 in 7434768
I don't know what would happen either if someone totally messes up system attributes by changing the scope or other properties of the attribute. But since it's possible to edit system attributes for the catalog_product entity, I think any changes preventing that should be in another PR. I'll knock a few other TODOs from my list, squash & force push this branch to clean it up some. |
7434768
to
3db964b
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
# Conflicts: # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Edit.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Edit/Form.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Edit/Tab/Main.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Edit/Tabs.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Grid.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set/Grid.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set/Main.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set/Main/Formattribute.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set/Main/Formgroup.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set/Main/Formset.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set/Main/Tree/Attribute.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set/Main/Tree/Group.php # app/code/core/Mage/Eav/Block/Adminhtml/Attribute/Set/Toolbar/Add.php # app/code/core/Mage/Eav/Block/Adminhtml/Helper/Form/Boolean.php # app/code/core/Mage/Eav/Helper/Data.php # app/code/core/Mage/Eav/Model/Entity/Attribute.php # app/design/adminhtml/default/default/template/eav/attribute/js.phtml # app/design/adminhtml/default/default/template/eav/attribute/set/main.phtml # app/design/adminhtml/default/default/template/eav/attribute/set/main/tree/attribute.phtml # app/design/adminhtml/default/default/template/eav/attribute/set/main/tree/group.phtml # app/design/adminhtml/default/default/template/eav/attribute/set/toolbar/add.phtml # app/locale/en_US/Mage_Eav.csv # docs/EVENTS.md
- Mage_Eav_Adminhtml_Attribute_AbstractController is not used new ... re-check later ...
Why? It was a simple question to @justinbeaty .... As I see you have another branch for the other project, So i'll contiunue here. It should not break anything. Will merge the simpler category-PR so we can start to test. edit2:
All new files dont name Magento ... the filese are newly added (and we have to use the given methods, so i have dropped that lines) |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Fishing PRs from one side to another is fine as far as I'm concerned, It's already practiced with success. However, I would appreciate if every author would honestly mention where he would like to implement, as happened here. In this way, the code will remain common between repos on certain features. I wish you success in the implementation and I hope to see it merged in OpenMage too. |
Description (*)
This PR gives backend users the possibility to create/edit/delete any EAV attribute. In other words, all of these:
I have only done light testing on this, we probably need more work before this can be merged as a useful feature.
Related Pull Requests
This PR builds on top of the work done by @fballiano. It is meant to supersede these PRs if the community accepts.
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Todo:
use_in_forms
option?adminhtml_[entityCode]_attribute_scopes
event to disallow changing scope perhaps?admin/XXX_attribute/new/
andadmin/XXX_set/add/
Contribution checklist (*)