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

Create a value object for a component record #14044

Merged
merged 4 commits into from Feb 24, 2017
Merged

Create a value object for a component record #14044

merged 4 commits into from Feb 24, 2017

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Feb 12, 2017

Summary of Changes

Create a new JComponentRecord value object representing the object loaded by JComponentHelper::load() for a component. Shift the logic for converting the record's parameters to a Registry object to the point where the parameters are first accessed (this removes a needless conversion if calling JComponentHelper::isEnabled() and that is the only interaction with a component's record).

The value object extends JObject as a bridge to help with the transition. At 4.0, this class extension should be dropped as well as the magic getter/setter methods.

Testing Instructions

The CMS still functions as intended. Component parameters are lazy loaded when needed.

Documentation Changes Required

Document the new class and scheduled deprecations.

*
* @since __DEPLOY_VERSION__
*/
class JComponentRecord extends JObject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this needs to extend JObject?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erred heavily on the side of caution since right now it's a stdClass object. Much of the same reasoning why the new JMenuItem does the same. The intent is in 4.0 to remove this.

If it's not needed I can drop it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I can buy it. In that case can we please add a @note in to reflect we are going to drop JObject property in J4 then please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do the same for XTD_buttons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgt41 Make your own PR 😛

@mbabker
Copy link
Contributor Author

mbabker commented Feb 13, 2017

FWIW I think we should make more liberal use of value objects versus untyped stdClass objects as stated in #13922 and not just for the lazy loading behaviors that have conveniently come here and the menu item's object. It also helps to make it clear exactly what properties you're getting on an object versus having to reverse engineer the code to find what is actually being loaded into memory and made available, so it makes it a little easier to jump into the code too. Developer Experience improvements FTW.

/**
* Class constructor
*
* @param array $data The menu item data to load
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look like copy/paste comment from somewhere. Should be The component record data to load or something like that

@joomdonation
Copy link
Contributor

I have tested this item ✅ successfully on 53a260f

1. Access to some pages in frontend, backend, creating new articles and all still working as expected

  1. Write some custom code to check and confirm that params is lazy load
    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14044.

@ghost
Copy link

ghost commented Feb 22, 2017

I have tested this item ✅ successfully on ff7e2c0

Click on different Menues in Frontend, append Menu and Modules.

Don't tested lazy load cause don't know how.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14044.

@ghost
Copy link

ghost commented Feb 24, 2017

RTC as there are 2 successfully Tests?

@jeckodevelopment
Copy link
Member

I have tested this item ✅ successfully on ff7e2c0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14044.

@jeckodevelopment
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14044.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 24, 2017
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Feb 24, 2017
@rdeutz
Copy link
Contributor

rdeutz commented Feb 24, 2017

Agreed on the concept, but because JObject is deprecated in 4.0, why we are doing it? Or will you remove the extension of JObject in 4.0 and keep the value object?

@mbabker
Copy link
Contributor Author

mbabker commented Feb 24, 2017

The extension of JObject is just to be extremely safe about it (we're converting from a stdClass to the value object). The extension goes away in 4.0 and the object stays.

@rdeutz
Copy link
Contributor

rdeutz commented Feb 24, 2017

ok, so I got the idea

@rdeutz rdeutz merged commit 2dd926a into joomla:staging Feb 24, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 24, 2017
@mbabker mbabker deleted the component-object branch February 24, 2017 19:31
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.

None yet

7 participants