magento-lts
magento-lts copied to clipboard
OpenMage
Allow for yes/no attributes to have an "empty" option.
Description (*)
When entering product details, all dropdown and multi-select attributes can be left "empty", except for the yes/no attributes. This is a pain, because sometimes you just don't know how to enter the value. You can't select "empty" or "maybe" or "I don't know". In practise, I've seen shopkeepers create dropdowns and enter Yes/No/Maybe options that way, but it's dirty.
If you really need to have a Yes/No attribute without the "empty" option, you can just make the attribute "Required", and that's the same thing as it was before. So it's not really a breaking change, unless you've configured your attributes poorly.
Adding the 2 extra arguments works fine, because getAllOptions() is already called with them when constructing the edit product form. See https://github.com/OpenMage/magento-lts/blob/242a99b643cbcf75c85310866557d821475ecfdd/app/code/core/Mage/Adminhtml/Block/Widget/Form.php#L208
If you like, we can move this to 20.x branch, I don't mind, but this is a big deal for data-conscious merchants.
Contribution checklist (*)
- [x] Pull request has a meaningful description of its purpose
- [x] All commits are accompanied by meaningful commit messages
- [ ] All automated tests passed successfully (all builds are green)
This PR is now complete and should work for all attributes regardless of frontend_input value boolean or select. The source_model for these attributes is Mage_Eav_Model_Entity_Attribute_Source_Boolean.
When editing a product, you can select "empty string", "yes" or "no". If the attribute is really needed, just set is_required to 1 and it should work as expected. If not required, you can fill in the empty value and it will save to the DB as NULL.
This was tested with value storage in the int EAV table.
Can please someone add some documentation about whats already different between 19.x and 20.x?
@sreichel I've also asked this question, but I don't have an answer. I'm just following what I've been asked. Not sure if this PR is the best place for this discussion.
are we sure this isn't going to create weird behaviours with exiting custom code? I'm thing about system->configuration for example.
As far as I understand this change refers to [Yes/No] input type of an attribute. Personally I do not agree with the change as it can be achieved very easily by using the input type [Dropdown]. I have stores where I had to use both [Yes/No] and a dropdown with Empty/Yes/No.
It should remain as it is!
@ADDISON74 for this reason you have the "Required" attribute setting. If required, you cannot leave empty. If not required, you can use the empty value.
An admin can turn is_required on or off, but (s)he CANNOT change the input type! My clients are advanced eCom sellers and manage attributes themselves.
While your suggestion works, it requires a DEV or someone with DB skills to change the type in eav_attribute.
Thank you for your clarification. I will test the commit and come back with a feedback.
Great! Looking forward to it. I am running this in production for 2 weeks now and it has worked as expected, but please don't take my word for it. I can always overlook something.
I changed those 3 files in a production server. Here is the error message I am getting when I try to edit a product in Backend:
Warning: array_unshift() expects parameter 1 to be array, null given in /home/mydomain/public_html/app/code/core/Mage/Eav/Model/Entity/Attribute/Source/Boolean.php on line 57
#0 [internal function]: mageCoreErrorHandler(2, 'array_unshift()...', '/home/mydomain/p...', 57, Array)
#1 /home/mydomain/public_html/app/code/core/Mage/Eav/Model/Entity/Attribute/Source/Boolean.php(57): array_unshift(NULL, Array)
#2 /home/mydomain/public_html/app/code/core/Mage/Adminhtml/Block/Widget/Form.php(201): Mage_Eav_Model_Entity_Attribute_Source_Boolean->getAllOptions(true, true)
#3 /home/mydomain/public_html/app/code/core/Mage/Adminhtml/Block/Catalog/Product/Edit/Tab/Attributes.php(70): Mage_Adminhtml_Block_Widget_Form->_setFieldset(Array, Object(Varien_Data_Form_Element_Fieldset), Array)
#4 /home/mydomain/public_html/app/code/core/Mage/Adminhtml/Block/Widget/Form.php(144): Mage_Adminhtml_Block_Catalog_Product_Edit_Tab_Attributes->_prepareForm()
#5 /home/mydomain/public_html/app/code/core/Mage/Core/Block/Abstract.php(937): Mage_Adminhtml_Block_Widget_Form->_beforeToHtml()
#6 /home/mydomain/public_html/app/code/core/Mage/Adminhtml/Block/Catalog/Product/Edit/Tabs.php(79): Mage_Core_Block_Abstract->toHtml()
#7 /home/mydomain/public_html/app/code/core/Mage/Core/Block/Abstract.php(311): Mage_Adminhtml_Block_Catalog_Product_Edit_Tabs->_prepareLayout()
#8 /home/mydomain/public_html/app/code/core/Mage/Core/Model/Layout.php(487): Mage_Core_Block_Abstract->setLayout(Object(Mage_Core_Model_Layout))
#9 /home/mydomain/public_html/app/code/core/Mage/Core/Model/Layout.php(503): Mage_Core_Model_Layout->createBlock('adminhtml/catal...', 'product_tabs')
#10 /home/mydomain/public_html/app/code/core/Mage/Core/Model/Layout.php(245): Mage_Core_Model_Layout->addBlock('adminhtml/catal...', 'product_tabs')
#11 /home/mydomain/public_html/app/code/core/Mage/Core/Model/Layout.php(211): Mage_Core_Model_Layout->_generateBlock(Object(Mage_Core_Model_Layout_Element), Object(Mage_Core_Model_Layout_Element))
#12 /home/mydomain/public_html/app/code/core/Mage/Core/Model/Layout.php(216): Mage_Core_Model_Layout->generateBlocks(Object(Mage_Core_Model_Layout_Element))
#13 /home/mydomain/public_html/app/code/core/Mage/Core/Controller/Varien/Action.php(360): Mage_Core_Model_Layout->generateBlocks()
#14 /home/mydomain/public_html/app/code/core/Mage/Core/Controller/Varien/Action.php(271): Mage_Core_Controller_Varien_Action->generateLayoutBlocks()
#15 /home/mydomain/public_html/app/code/core/Mage/Adminhtml/Controller/Action.php(290): Mage_Core_Controller_Varien_Action->loadLayout(Array, true, true)
#16 /home/mydomain/public_html/app/code/core/Mage/Adminhtml/controllers/Catalog/ProductController.php(260): Mage_Adminhtml_Controller_Action->loadLayout(Array)
#17 /home/mydomain/public_html/app/code/core/Mage/Core/Controller/Varien/Action.php(437): Mage_Adminhtml_Catalog_ProductController->editAction()
#18 /home/mydomain/public_html/app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(262): Mage_Core_Controller_Varien_Action->dispatch('edit')
#19 /home/mydomain/public_html/app/code/core/Mage/Core/Controller/Varien/Front.php(192): Mage_Core_Controller_Varien_Router_Standard->match(Object(Mage_Core_Controller_Request_Http))
#20 /home/mydomain/public_html/app/code/core/Mage/Core/Model/App.php(381): Mage_Core_Controller_Varien_Front->dispatch()
#21 /home/mydomain/public_html/app/Mage.php(729): Mage_Core_Model_App->run(Array)
#22 /home/mydomain/public_html/index.php(78): Mage::run('', 'store')
#23 {main}
I put back the original /app/code/core/Mage/Eav/Model/Entity/Attribute/Source/Boolean.php file and was able to load the product edit page.
I will come back with an opinion related to the modification of this type of attribute but after analyzing all the aspects involved.
@ADDISON74 I did not run into any issues and using this code for many months now. Must be something on your end I believe.
@woutersamaey - in the end it was an error in the code not in my test environment. trust me I am doing intensive tests when I report issues. If you had the change in production you would have noticed when editing a product that there are problems. Most likely you did not edit any product after the change.
As it is now OpenMage if you use an attribute of type Yes/No the form will send one of the two values, regardless of whether it is a required attribute or not because there is always a selected value. In order to obtain flexibility in the choice, another value can be entered (e.g. Empty) then it is chosen to create another attribute of dropdown type.
Therefore, Magento has a strict Yes/No attribute. If a new Empty value is added then a problem arises. How can I perform the initial Yes/No functionality again? Obviously by creating a dropdown attribute! I mean exactly what I was doing now, but the other way around. I practically didn't solve anything
In addition, I did not do a test to verify the implications of existing attributes such as Yes/No and which will behave as being modified by this change.
I'm sorry but I don't agree with this change. Yes/No is a strict attribute type for a particular situation derived from dropdown. If Empty is added to it, it loses its original utility as it was conceived. I can instead agree with a new attribute of type Yes/No/Empty, but in no case with the modification of the original Yes/No.
@ADDISON74 I did indeed create this PR too quickly, but have the logic in production.
You seam to be forgetting about the required field which forces 1 of 2 options. If you disable required, you can have 3 options: NULL, yes and no.
If the attribute has required turned on, you cannot use the NULL option as the frontend validation will prevent you from saving so your concern is not really valid anymore.
A breaking change can occur, if you have wrong required values set on your attributes, but I cannot solve that from my end. Shopkeepers need to properly configure their attributes on their own.
@woutersamaey - Thank you for the explanations.
The fact that when it is a required attribute only two Yes/No options remain does not change the actual situation too much. Maybe if I made a store from scratch this PR would be fine but in the case of an existing store all the attributes of type Yes/No must be checked and set as required.
In this case we really have a problem, obviously it can be solved extremely easily by modifying them in the database, but how many will know how to do it and especially if they encounter problems where to address? I like this change but I do not want to bring new problems with it. That's why I proposed a Yes/No/Empty attribute to make it easier for those who do it from the dropdown attribute. I am one who think I have in a store over 300 such attributes.
@ADDISON74 Can we do an upgrade script where we make all boolean attributes required? This would make sure there are no breaking changes and we can move this along.
The NULL option is actually really important for content teams who are entering products and are not sure of the value they should enter. A simple example can be for a lighting fixture. Is there a bulb included with the product? Yes, no or not sure...
The dropdown type does offer a way out, but it is very ugly. I have a client with hundreds of these types of attributes and cleaning this up is a terrible task. Thanks to this change, we were able to do it, and everyone's happy.
Let's evaluate in more detail this change and the implications it has in stores that already run OpenMage.
When I create a new Yes/No attribute for the "Values Required" option the initial value is set to No. If this value is changed to Yes then not much changes because the list, whether you choose Yes or No, will always set a value . It's not like when you use a Text Field attribute, you don't enter a value and you're asked to enter one.
There is also the situation when I want a new value in the list, for example "nothing" an empty string. In this case I create an attribute Dropdown with the values in the desired order, for example nothing/No/Yes. If I chosen to be a required attribute whether or not an option is chosen, there will still be a value that results from the list, it could be "nothing". The difference from Yes/No attribute is that we also have the absolutely necessary Empty option when there should be a restrictive option as Yes/No. As I said before I have in a store more than 300 attributes of this type.
Now let's evaluate your PR and the benefits it brings. The actual Yes/No attribute is modified to offer two alternatives Yes/No and Yes/No/Empty depending on the option of the attribute as requested or not.
-
What happens if there are already attributes initially created as Yes/No and which are either required or not? I exclude the use of an automatic script because in such a situation an attribute-by-attribute evaluation is required.
-
In the case of Yes/No attribute even if it was not requested, the list produces always a value. What happens if the visitor does not change the Empty/Yes/No list and I would like to get a value like before Yes or No? In order to fix this I will create a dropdown attribute with two values Yes/No. I practically turned around the tail.
I remain of the opinion that each has been well analyzed by the Magento team.
-
The Yes/No attribute is a strict one and the list provides a value even if it is requested or not.
-
If I want one or more options then the Dropdown attribute offers flexibility, including positioning options in the list.
Practically, each attribute has its advantages and disadvantages. Through this PR you want to combine them to get a facility in one of them but you have to use Dropdown attribute to achieve Yes/No attribute as it is now.
Any other opinion than mine is welcome.
Using dropdown for Yes/No is a terrible solution as you need to translate Yes/No for each language manually and repeat for 100s of attributes. This causes a huge overhead for what is in fact a boolean with null support.
I want OpenMage to move forward, not keep or repeat bad design from Magento 10 years ago.
So we disagree then.
I didn't test the PR but I support the change in 20.x in general. The boolean-attribute has always been problematic. In many cases its not what you want because it does not have a null option and even if you always want a value, it's a source for errors, because there is no way to enforce that a value must be actively chosen by the user in the BE. IMHO this PR does enhance the functionality in general and usability in the BE. I never liked the fact that you can forget to set a boolean attribute to yes and end up with no because the validator cant tell you...
I have the class Mage_Eav_Model_Entity_Attribute_Source_Boolean as source_model quite often in my projects. Except for this class, all other source models have an empty option in the dropdown. So, it is inconsistent, but it's a feature. The feature is that the default value of the attribute is No.
@woutersamaey issue is that he doesn't want to have a default value. An easy solution is to make a custom source_model and use that, instead of Mage_Eav_Model_Entity_Attribute_Source_Boolean. Or add additional class Mage_Eav_Model_Entity_Attribute_Source_YesNoEmpty.
Adopting this PR does not affect me because in all stores I manage I have recommended from the very beginning the use of Dropdown for Yes/No which ensures greater flexibility and avoids confusion. If it were up to me I would definitely give up the Yes/No attribute which is nothing but a particular situation of the Dropdown attribute but which offers me facilities for both positioning and validation, an aspect that does not happen in the case of Yes/No where all time there is a selected value whether it is required or not.
If it is merged regardless of the OM variant in which 19 or 20 goes all its implications must be analyzed. In the last year the request for starting an online store with Magento 1 decreased a lot and such changes generally go to stores that already run Magento/OpenMage. For sure they have Yes/No attributes in sets, maybe requested or not for a selected value.
@kiatng - what you propose is exactly what I recommended in one of the posts above. In order to reconcile everyone, not creating issues with the already existing attributes in sets, a new class of the Yes/No/Empty attribute is a good idea. Such an attribute will really help me which so far I created from the Dropdown attribute. If this approach is desired then it is only necessary to analyze what happens in the situation when the attribute is requested and when not.
After all this discussion, I still feel my PR is valid and offers a big improvement. Why else is there a "Required yes/no" option on an attribute if we cannot use it here? Can we still merge this please?
Turns out that with today's code only the cast to string is needed. Everything else is not needed anymore. If we do not cast to string, the "No" value is selected instead of the empty value.
So everything I mentioned here is today no more than a UI bug...
@woutersamaey - I am still waiting for your opinion on the above objections posted by me and @kiatng.
@ADDISON74 your question was already addressed from the beginning. If you want a strict yes/no attribute and do not want an empty option, you should make the attribute "required". This forces the admin to pick yes or no.
Basically all cases are handed:
- Yes/No = required on
- Yes/No/Empty = required turned off
If you look at the code changes it's nothing more than fixing an in_array() condition that cannot differentiate between empty string and "0". So a UI bug really.
Everything else seams to have already been solved though other updates and recent changes.
I appreciate the clarifications you have made. I will test again the PR because I noticed there are changes & fixes after our conversation which is almost 1 year old.
What I find interesting is that this PR has 6 commits but in the "File changed" section only one file appears to be modified.