Fix "Store View" field value empty when reloading form after failed save. #415
Conversation
|
Fix works. Are there any downsides with fixing the field name? Why is it |
| @@ -102,6 +102,7 @@ public function editAction() | |||
| // 3. Set entered data if was error when we do save | |||
| $data = Mage::getSingleton('adminhtml/session')->getFormData(true); | |||
| if (! empty($data)) { | |||
| $data['store_id'] = $data['stores']; | |||
There was a problem hiding this comment.
If the form is using stores[] as the element name then it's meant to be an array. Here you seem to just be grabbing the array (or possible array) and assigning it to a field that's meant to be a scalar, no?
There was a problem hiding this comment.
Um, store_id i not meant to be a scalar. Think these lines from #382 make it more clear ...
# is delete or edit action?
$isDeleteAction = $observer->getEvent()->getName() === 'cms_page_delete_before';
$storeIds = $isDeleteAction ? $page->getStoreId() : $page->getStores();
There was a problem hiding this comment.
I disagree.
The underlying model resource clearly expects that store_id is a scalar value that is used as a fallback when stores is not present. The code example you listed is faulty.
Edit: Faulty in the fact that if someone sets store_id to a scalar then it'll likely cause issues since the code shown expects that it's an array.
There was a problem hiding this comment.
And I'll follow up with this example that is contradictory to what I just said but which I consider a bug.
store_id is clearly singular and in every other context on other models is a scalar. They clearly expect it to be a scalar on save since they cast it to an array. The fact that after loading they assign the array of store IDs to store_id vs. stores is, IMHO, 100% a bug in the core code.
There was a problem hiding this comment.
Further evidence that the core code is buggy regarding the treatment of store_id: https://github.com/OpenMage/magento-mirror/blob/magento-1.9/app/code/core/Mage/Cms/Model/Resource/Page.php#L203-L204
Here the object loading code clearly expects store_id to be a scalar, not an array.
There was a problem hiding this comment.
IMHO, 100% a bug in the core code.
Thats what i've meant. :)
There was a problem hiding this comment.
The input name is stores because that is the data field that is expected when saving a page. The fact that the core afterLoad code is dumping the store IDs into store_id instead of stores is clearly a bug. There is no DB field for store_id of a page (directly). There is an additional link table that links pages to stores so a page has an array of store ids always.
There was a problem hiding this comment.
Isn't it posible to add an alias to cms_page_store.store_id as stores?
(But then you need a fallback to store_id, at least for 3rd party code.)
There was a problem hiding this comment.
I would say adding stores in the after load is a good first step in the overall issue.
There was a problem hiding this comment.
Adding $object->setData('stores', $stores); to _afterLoad solves only my problem (different getters for store ids), but it doesnt solve the problem @colinmollenhour describes.
To reproduce ...
- add an observer to
cms_page_save_before- - just throw an exeption to display an error message
Selected stores are still empty.
|
@sreichel They tend to use |
|
Think this fix is good as it is. Fixing |
... and what happened to this post? Has that problem been solved or not? |
The form uses 'store_id' as the element id but 'stores[]' as the input name since the model after save uses
getStoresfor the new store ids. Without this change the store_id is empty and so the Store View field values get wiped when the form is reloaded.