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

Use collection _idFieldName by default for toOption* methods. #182

Closed

Conversation

colinmollenhour
Copy link

This change allows the toOptionArray and toOptionHash methods work for resource collections without having to override these methods for every instance.

@magento-team
Copy link
Contributor

Hello, thanks for the suggestion. We have discussed it but we are rather against changing the abstract super-classes. Can you elaborate what is the primary use of your change?

@colinmollenhour
Copy link
Author

The Varien_Data_Collection class uses 'id' as the default $valueField parameter but for super-classes of Varien_Data_Collection_Db the fieldname 'id' is usually not correct. Instead the field is something like entity_id or user_id, etc.. So, with these super-classes the toOptionArray and toOptionHash methods have to be overridden just to specify a different $valueField. Since the id field name is already known (getIdFieldName()), it makes sense to make it the new default rather than just 'id'. Then in a large number of cases the methods will not need to be overridden and they can be used as-is.

So the primary use of the change is just to reduce the need to override methods where it would otherwise not be necessary as the toOptionArray and toOptionHash methods are useful for many collections.

Example:

$categoryNames = Mage::getResourceModel('Mage_Catalog_Model_Resource_Category_Collection')->toOptionHash();

The above does not work before my patch and should work after my patch.

magento-team added a commit that referenced this pull request Nov 22, 2013
* Moved general action-related functionality to \Magento\App\Action\Action in the library. Removed Magento\Core\Controller\Varien\Action and related logic from the Magento_Core module
* Moved view-related methods from action interface to \Magento\App\ViewInterface with corresponding implementation
* Moved redirect creation logic from the action interface to \Magento\App\Response\RedirectInterface
* Moved Magento\Core common blocks to the library
* Added reading of etc/integration/config.xml and etc/integration/api.xml files for API Integrations
* Various improvements:
  * Email-related logic from the Core and Adminhtml modules consolidated in the new Email module
* GitHub requests:
  * [#238](#238) -- Improve escaping HTML entities in URL
  * [#199](#199) -- Replaced function calls to array_push with adding the elements directly
  * [#182](#182) -- By default use collection _idFieldName for toOption* methods.
  * [#233](#233) -- Google Rich Snippet Code
  * [#339](#339) -- Correcting 'cahce' typo in documentation.
  * [#232](#232) -- Update app/code/core/Mage/Checkout/controllers/CartController.php (fix issue #27632)
* Fixed bugs:
  * Fixed JavaScript error when printing orders from the frontend
  * Fixed Captcha problems on various forms when Captcha is enabled on the frontend
  * Fixed "Page not found" on category page if setting "Add Store Code to Urls" to "Yes" in the backend config
  * Fixed Fatal error when creating shipping label for returns
@verklov
Copy link
Contributor

verklov commented Nov 25, 2013

Hello colinmollenhour,
We have processed your pull request. The code should be available in the code version dev53 released last Friday.

Thank you for your contribution!

@verklov verklov closed this Nov 25, 2013
@colinmollenhour colinmollenhour deleted the tooption-default branch November 25, 2013 14:55
vpelipenko added a commit that referenced this pull request Mar 23, 2015
okorshenko pushed a commit to isitnikov/magento2 that referenced this pull request Aug 10, 2016
[Support] Portdown MAGETWO-50026 down to M2.0.x branch
VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this pull request Nov 25, 2017
This PR fixes the SalesChannelManagement web api test issue due to wrong usage of fixtures.
VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this pull request Nov 25, 2017
magento-engcom-team added a commit that referenced this pull request Oct 4, 2018
 - Merge Pull Request magento/graphql-ce#182 from magento/graphql-ce:graphql-128-extends-store-config-coverage
 - Merged commits:
   1. 8f3dd40
taskula pushed a commit to Hypernova-Oy/magento2 that referenced this pull request Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants