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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#33353] JUcmContent/base getType() should return same instance #3194

Closed
wants to merge 2 commits into from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Feb 27, 2014

getType() should return same Type instance instead of make new each time

not very know how to test it, but for me it obvious mistake 馃槃

Joomla! Issue Tracker [#33353] JUcmContent/base getType() should return same instance

@Bakual
Copy link
Contributor

Bakual commented Feb 27, 2014

Is there always the same type requested on a pageload? Or what happens if a page needs multiple types?
I'm not sure where this is used exactly, so you at least have to provide an issue (like repeatingly calling same getType($alias) ) where we can test it.
Also a tracker item would be needed.

@Fedik
Copy link
Member Author

Fedik commented Feb 27, 2014

sorry forgot to add the tracker link, fixed

I found it used on save the Article with Tags, I think it used only for initialise Type instance on JUcmBase __construct, but not sure

I think it can make some hidden bugs, because if you do

$ucm = new JUcmBase('alias', $type_istance);
//and then
$ucm->getType()

it will return new instance each time.

so I think getType() should be protected or should return same instance,
and as I have no idea where it used in Joomla, I made second thing 馃槃

@wilsonge
Copy link
Contributor

I agree with this fix. However I disagree with the change made in the constructor in JUcmBase (obviously calling the parent method is correct in the subclass). The thing we have now is much clearer

@Fedik Fedik changed the title JUcmContent/base getType() should return same instance [#33353] JUcmContent/base getType() should return same instance Aug 21, 2014
@wilsonge wilsonge mentioned this pull request Mar 17, 2015
@wilsonge
Copy link
Contributor

Closed in favour of #6466 which is the same thing but without the constructor codestyle change I asked to be reverted

@wilsonge wilsonge closed this Mar 17, 2015
@Fedik Fedik deleted the fix-jucmcontent-gettype branch March 24, 2015 17:36
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

Successfully merging this pull request may close these issues.

None yet

5 participants