PHP 7.2 fixes (method signature)

[ghost]

PHP 7.2 requires method signatures to match when a class overrides a method from the parent. This PR fixes the getAllOptions method in classes that extend Mage_Eav_Model_Entity_Attribute_Source_Table.

In each instance, the child method used no parameters, thus including them will not break anything. There may be other places where this is a problem, and I will fix as I come across them.

[ghost]

fixes #449

[colinmollenhour]

Since the new arguments have no effect perhaps they should be renamed or documented to make it clear that using them will have no effect?

[ghost]

Added phpdoc info. I think it's probably cleaner than naming the arguments $doNotUse1 and $doNotUse2 unless you had a better idea?

[tmotyl]

I don't care that much about argument names, as modern IDEs has visual indication that argument is not used. So having doc comment is good enough for me.

[Flyingmana]

I prefer to keep the argument names the same, in case other extended classes want to re-add functionality there

[ghost]

Before this is merged, should we change the $withEmpty default option to false? This is what the code looks like in Mage_Eav_Model_Entity_Attribute_Source_Table. Whether we implement this code in the children methods is another question, as it isn't likely to be used anywhere.

        if ($withEmpty) {
            array_unshift($options, array('label' => '', 'value' => ''));
        }

Edit: see below, but changing the default value won't do much, since these methods do in fact get called with values for $withEmpty and $defaultValues

[ghost]

So for example, the method in file app/code/core/Mage/Customer/Model/Customer/Attribute/Source/Group.php would look like this:

public function getAllOptions($withEmpty = false, $defaultValues = false)
{
    if (!$this->_options) {
        $this->_options = Mage::getResourceModel('customer/group_collection')
                        ->setRealGroupsFilter()
                        ->load()
                        ->toOptionArray()
                        ;
    }
    $options = $this->_options;
    if ($withEmpty) {
        array_unshift($options, array('label' => '', 'value' => ''));
    }
    return $options;
}

Edit: I just tried this out, and it seems that the getAllOptions does get called with true from app/code/core/Mage/Adminhtml/Block/Widget/Form.php

                if ($inputType == 'select') {
                    $element->setValues($attribute->getSource()->getAllOptions(true, true));
                } else if ($inputType == 'multiselect') {
                    $element->setValues($attribute->getSource()->getAllOptions(false, true));
                    $element->setCanBeEmpty(true);
                }

Thus the above change I suggest would break things. It maybe could be fixed like this:

                if ($inputType == 'select') {
                    if (get_class($attribute->getSource()) == 'Mage_Eav_Model_Entity_Attribute_Source_Table') {
                        $element->setValues($attribute->getSource()->getAllOptions(true, true));
                    } else {
                        $element->setValues($attribute->getSource()->getAllOptions(false, false));
                    }
                } else if ($inputType == 'multiselect') {
                    $element->setValues($attribute->getSource()->getAllOptions(false, true));
                    $element->setCanBeEmpty(true);
                }

But I'm not positive that Mage_Eav_Model_Entity_Attribute_Source_Table and its sub-classes are the only things being called upon by $attribute->getSource()

[ghost]

So, after looking at all this, I'd recommend keeping the PR as it is, and if the community decides to implement the functionality of $withEmpty and $defaultValues for whatever reason, then we will need to dig into app/code/core/Mage/Adminhtml/Block/Widget/Form.php and maybe other files as well. But I don't really see a need to implement the extra functionality as it isn't used anywhere.