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

Developer experience: getData should log warnings when an attribute is not loaded #362

Open
amenk opened this issue Mar 26, 2020 · 4 comments

Comments

@amenk
Copy link

amenk commented Mar 26, 2020

Is is a common issue to try to fetch data from objects like products with $product->getData('attribute') but depending on the source of those objects (for example if they come from an collection) the attribute is not loaded.

I would suggest to at least add a warning

if (!$this->hasData('attribute')) {
   $logger->warning('Trying to access not existing / not loaded attribute');
}

This would improve the developer experience.

@ihor-sviziev
Copy link

ihor-sviziev commented Mar 27, 2020

@amenk
Could you describe your use case in which it could be useful?

From my perspecitve in many cases we're just using $this->getData('attribute') ?: 'default value' and it will be really bad experience to have warning in this case.
Also for production having warnings will dramatically increase amount of logs and will not bring any value.

For now I put there 👎.

@amenk
Copy link
Author

amenk commented Mar 27, 2020

Thanks for your quick feedback - I absolutely get your point.

There are lots of cases where you need a certain attribute, but depending on which product instance you have (in an observer for example) and were it was loaded, some attributes might not be available. This is a common source for errors. The attribute is in fact in the database, but just not loaded.

What do you think about something like $object->getDataOrFail() so it would be fully backwards compatible. So that one could even throw an expection if the data is not loaded and it's the choice of the developer to state "I really need to have this attribute available, otherwise wrong logic might be triggered".

@ihor-sviziev
Copy link

ihor-sviziev commented Mar 27, 2020

Hi @amenk,

What do you think about something like $object->getDataOrFail() so it would be fully backwards compatible.

Unfortunately it will not be backward compatible change and class is marked with @api tag.

As a workaround in place where you need it (in your customizations) - you could add after plugin for getData method and implement whatever logic you need.

@amenk
Copy link
Author

amenk commented Mar 27, 2020

I know there are workarounds to solve this locally, I raise this topic here in architecture to see if it is a good fit for general advancement of the Magento platform.

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

No branches or pull requests

2 participants