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

[6.0] Unify loadFormData() to return an object #44148

Open
wants to merge 7 commits into
base: 6.0-dev
Choose a base branch
from

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Sep 27, 2024

Summary of Changes

We are in the process of migrating the core away from CMSObject and in that process I noticed that we are very inconsistent with the data that is returned from loadFormData(). There are cases where this is an array and cases where this is an object, both from the same method. Most code is also confused what it is supposed to be, array or object. This PR unifies this to an object and removes the need for CMSObject.

Testing Instructions

Test all the edit views of the extensions touched by this PR.

  • OR -

Codereview.

Expected result AFTER applying this Pull Request

Nothing should have changed.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@Fedik
Copy link
Member

Fedik commented Sep 28, 2024

I am sorry, this not going to work.
It is should be an opposite actually, and I think we talked about opposite.
The result of loadFormData() should be always array.

@Hackwar
Copy link
Member Author

Hackwar commented Sep 28, 2024

@Fedik Right now in 90% of the cases, you get a CMSObject in return, so this is more correct than before. All our code either expects an object or converts to an object. Array has been superseded by reality here effectively.

@Fedik
Copy link
Member

Fedik commented Sep 28, 2024

yes, and no :)
It not about what our code expecting, but about how to make it "right".

Look, this looks not good (and other similar places):

protected function loadFormData()
{
return new \stdClass();
}

This will break everything where we have if (empty($data))

The method loadFormData() were set to return array since begining of its existance. And that our Models return object (in some cases) it is bug in that models.

I do not know how to resolve all of these, but personally, I think in conext of Form it still should return array, as it more easy to work with, also it prevents the data "referencing" (because objects always pased by reference).

@richard67
Copy link
Member

Look, this looks not good (and other similar places):

protected function loadFormData()
{
return new \stdClass();
}

This will break everything where we have if (empty($data))

I agree with @Fedik that it would be better to unify to array, but if object then we maybe should return null in such cases like mentioned above and not an empty object.

@Hackwar
Copy link
Member Author

Hackwar commented Sep 29, 2024

4 out of 43 models in our system return an array instead of an object.

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.

5 participants