Skip to content

Protected getters and setters should be public #10792

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

Conversation

VincentMarmiesse
Copy link
Contributor

Hello,

These getters and setters should be public and not protected function, otherwise we can't access to the class params.

@orlangur orlangur self-assigned this Sep 6, 2017
@orlangur
Copy link
Contributor

orlangur commented Sep 6, 2017

As far as I can see from implementation they are not supposed to be used directly but only via \Magento\Catalog\Helper\Image::init method. Please provide a scenario when it is not enough.

@VincentMarmiesse
Copy link
Contributor Author

Hello @orlangur,
I wanted to add a Plugin on the getLabel method and retrieve the image's product inside my Plugin.

@orlangur
Copy link
Contributor

orlangur commented Sep 6, 2017

Necessary product data can be intercepted in init method

@VincentMarmiesse
Copy link
Contributor Author

But how can I retrieve the product if I create a Plugin on the getLabel function? The init method won't help me here right?

@orlangur
Copy link
Contributor

orlangur commented Sep 6, 2017

For example, such plugin would intercept both init and getLabel methods and store intercepted product instance internally.

@VincentMarmiesse
Copy link
Contributor Author

Oh ok. But do you agree it's not really intuitive and practical to use? :)

@orlangur
Copy link
Contributor

orlangur commented Sep 6, 2017

It is quite straightforward if one notice that init method is the only way to change image helper state :) Still better than breaking class encapsulation and making its state hardly predictable by exposing unnecessary setters.

Are there any other customization problems which cannot be solved in current implementation?

@orlangur
Copy link
Contributor

orlangur commented Sep 6, 2017

Another question... Are you trying to customize behavior of some core code or it's a custom implementation?

@VincentMarmiesse
Copy link
Contributor Author

It's a custom implementation, it want to calculate the image label with some product information instead of just displaying it.

@orlangur
Copy link
Contributor

orlangur commented Sep 6, 2017

In such case you have full control on how and when image helper is called and plugin creation would be a misuse of interception mechanism.

Just encapsulate logic in some class so that you do

function ...
{
    $product->setSomeLabel(...);
    $imageHelper->init($product, ...);
}

or even not rely on getLabel from helper at all.

@orlangur
Copy link
Contributor

orlangur commented Sep 6, 2017

Let's wait for some alternative opinions a bit. I'm pretty sure we should not expose setters but probably public getProduct may appear to be useful for some other customization scenarios.

@VincentMarmiesse
Copy link
Contributor Author

Ok so in your opinion, I should not use a plugin here? I thought it was exactly the perfect scenario to use one.

@orlangur
Copy link
Contributor

orlangur commented Sep 7, 2017

@VincentMarmiesse yeah, plugin is best suitable when you need to customize some core behavior to your needs. In this case while technically possible it is better to reuse core functionality differently, aggregating image helper in your new class instead of pluginizing it and simply put custom getLabel implementation somewhere else.

@orlangur
Copy link
Contributor

@VincentMarmiesse thanks for collaboration! Hope suggested implementation approach will work like a charm for you leaving image helper methods with their current visibility (the stricter = the better).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants