Fix Inconsistency

[srenon]

Fix Inconsistency between $this->buttonList->add(...) and $this->addButton(...).

See /app/code/Magento/Backend/Block/Widget/Container.php

   public function addButton($buttonId, $data, $level = 0, $sortOrder = 0, $region = 'toolbar')
    {
        $this->buttonList->add($buttonId, $data, $level, $sortOrder, $region);
        return $this;
    }

[Vinai]

How about also replacing $this->buttonList->remove() calls with $this->removeButton()? That would further increase the consistency of the class.

We will have to wait for the travis build infrastructure to be fixed for the tests to pass. Once that happens we will be able to further process your PR.

[srenon]

@Vinai ... Good catch, my main focus at the time was trying to figure out how to add a button in sales order view. See http://magento.stackexchange.com/questions/91071/how-to-add-a-custom-button-to-admin-sales-order-view-in-magento2

[srenon]

@Vinai ... After reviewing Magento\Sales\Block\Adminhtml\Order\View.php

Using $this->buttonList->add/remove seem to be more consistent, because it is use 14 times while $this->addButton was only use 2 times.

But if you take a look at \Magento\Backend\Block\Widget\Form\Container (extended by View ) they are using $this->addButton()

And if you take a look at \Magento\Backend\Block\Widget\Container (extended by Container) this is where addButton() and removeButton() functionality was implemented.

So maybe the solution here is to stop using $this->buttonList which would make this pull request irrelevant.

[Vinai]

Well, consistency in this case only means doing things in the same way - not which way.
I think using a protected property of a parent class is a lot more risky then using calling protected methods provided by a parent class. Using protected properties is not really better then using global variables. At least it exposes the same risks. It breaks encapsulation.
So if I where writing the Order\View class I'd either depend on my own instance of the button list in my constructor, or I'd only call the parent methods to manipulate the parents button list instance.
But of course for whatever reason (maybe legacy) it's a fact that in Magento protected properties are used by child classes all over the place. So who am I to argue against it.

[srenon]

As i mention before my main focus was trying to add a button to the toolbar and notice this inconsistency without review the parent classes (and maybe we already spending too much time on this).

But after reviewing all the classes, the current way to do this is $this->updateButton(), $this->removeButton(), and $this->addButton() since these are public method and $this->buttonList is protected.

So the question is, do we want to make Order\View consistent, all the files consistent or just leave it alone

[Vinai]

Leaving code alone makes it worse :)
If you ask me, please update the PR so the parent methods are used. I can't promise it will be accepted (not my call), but I think that would be the right thing to do.

[vancoz]

Hello @srenon, please merge latest from develop and rerun builds.

[srenon]

I'm reopening this because I accidentally delete the branch

[NadiyaS]

Hi @srenon ,
thank you for contribution and we are sorry for delay.
Internal ticket MAGETWO-52647 was created to process your PR.

[ishakhsuvarov]

@srenon Thanks for contributing!