I would almost argue that the fact you aren't calling parent::__construct() is the problem. Otherwise, there is an argument to be made that every class property must be checked like this, which basically defeats the point of having a constructor.

@joeforjoomla is this Pull Request for #18349?

Yes @franz-wohlkoenig

@joeforjoomla

surely this is a difference, and good to bring this up (so that people are aware of this)
but the fix should be made in the child class that extends BaseController

i will check my code and if i find this in my own code , i will consider my own code having a bug
and i will fix my code

when extending a parent class and calling parent class methods,

• these methods assume that the constructor of their class has been called !

some languages do not even allow to skip it (if you skip it they will call it anyway)

the call to the parent constructor is there to be able to pass different arguments,
(if the parent constructor can accept arguments)

the fact that PHP allows skiping it, is something that should not be misused

-- but if skipping parent constructor of base controller is common in many extensions then this ??
then this PR should be considered to be accepted ?

Agree here with @mbabker and @ggppdk. We added a check in the constructor https://github.com/joeforjoomla/joomla-cms/blob/bbf9bce900256034eb1adee0c0aeee60d7078d46/libraries/src/MVC/Controller/BaseController.php#L442, if the factory doesn't exist, then it creates it. So not calling the parent constructor is for me the bug here.

I agree guys.
However the fact that out there a lot of extensions like mine could do this 'bugged' behavior not calling the parent constructor but still working on 3.8, could simply not be broken with this PR.
It can be safely removed at 4.0
Otherwise it means update for all extensions, a lot of Joomla websites broken, a lot of angry users, etc
Why not avoid all of this?

Thanks to @joeforjoomla for poiting this out.

IMHO this is a real huge problem and this PR is a life saver.

I built all my extensions on my own MVC framework since 3.1 branch and they are all affected on 3.9 because of this issue.

The B/C promise should be mantained for all 3.x versions even if a sort of bug code must continue to be accepted and work.

I'm aware that a lot of Joomla extension developers built their own frameworks for MVC that could be broken because of this.

@markgutton so you are extending the base controller class without calling the parent constructor? Why are you doing this? There is really no reason to not call the parent constructor. PHP should really force devs to do so.

I think this is a quite common behavior while extending JControllerLegacy by devs. Calling the constructor leads to issues in several cases where you want more flexibility on naming MVC classes.
By the way given that PHP allows it, i would not even mark at all this coding style as a bug.

Should we then not try to fix the issues instead of taking into account a very error prone behavior by extension devs.

I think this is a quite common behavior while extending JControllerLegacy by devs.

Again what forces you to do this? Perhaps I miss here something but for me there is absolutely no reason to avoid calling the parent constructor.

Well i don't think that's the question is to find a reason to call or not the parent constructor.
IMHO the question is to not break compatibility in the 3.x branch and speaking from your point of you, continuing to allow 'bugged' behavior to work as of now.
I think it's better to avoid breaking websites because of no more compatible extensions rather than ignore the problem suggested by this PR.

Thanks for your explanation @markgutton
I found an even worst case. If for example a system plugin was triggering a component MVC extending JControllerLegacy with an overridden constructor, the whole website would result inaccessible.
A user should investigate which extension is causing this to recover his Joomla website. It's not worth to risk. My PR does not hurt anything while ensure that things won't break.

Simply speaking:

because i know that until Joomla 3.8 there are extensions extending JControllerLegacy without calling the constructor, in the new BaseController class before using 'factory' property i check if it exists or not.
If not i assume a legacy extension and i assign an instance of LegacyFactory. Nothing more.

I don't find a reason why this should be wrong.

@joeforjoomla i fully agree with you.

Most important, because PHP allows to extend a base class and fully override a constructor without calling the parent one, and because there could be plenty of reasons for doing this, i would not consider this a bug or a bad practice, simply a design choice up to developers.

It's the duty of the Joomla CMS continuing to allow it if there is a known B/C break.

Almost all 'big extensions' out there are extending with its own controller the base class JControllerLegacy.
Just found one more, for example Hikashop.

This is the code of the controller used by Hikashop that only optionally calls the parent contructor:

class hikashopController extends hikashopBridgeController {
	var $pkey = array();
	var $table = array();
	var $groupMap = '';
	var $groupVal = null;
	var $orderingMap ='';

	var $display = array('listing','show','cancel','');
	var $local_display = array();
	var $modify_views = array('edit','selectlisting','childlisting','newchild');
	var $add = array('add');
	var $modify = array('apply','save','save2new','store','orderdown','orderup','saveorder','savechild','addchild','toggle');
	var $delete = array('delete','remove');
	var $publish_return_view = 'listing';
	var $pluginCtrl = null;

	function __construct($config = array(), $skip = false) {
		if(!empty($this->pluginCtrl) && is_array($this->pluginCtrl)) {
			if(!HIKASHOP_J16)
				$config['base_path'] = JPATH_PLUGINS.DS.$this->pluginCtrl[0].DS;
			else
				$config['base_path'] = JPATH_PLUGINS.DS.$this->pluginCtrl[0].DS.$this->pluginCtrl[1].DS;
		}
		if(!$skip) {
			parent::__construct($config);
			$this->registerDefaultTask('listing');
		}
		if(!empty($this->local_display))
			$this->display = array_merge($this->display, $this->local_display);
	}
	function listing(){
		JRequest::setVar('layout', 'listing');
		return $this->display();
	}
...

Do you really want to potentially break dozens of extensions?

Essentially right now what you're arguing is that we cannot change the core API to introduce new class member variables that are assigned through constructors. And IMO that's not an acceptable way to manage an API. It's almost as bad as saying we can't add methods to classes because extensions overload the core classes and sites break when users update without those extensions being updated (some people have actually suggested that and that one is an issue that exists with almost every one of our .0 releases).

The base class expects that it is fully configured through the constructor. If you are not calling the constructor and not configuring everything in the base constructor (either with the same logic that exists or your custom logic if you really care to have something custom), then you are leaving the door open to there being issues in the use of methods from the base class which then begs to question why you're even extending the base class if you aren't using it or the methods it provides (because there is no requirement for your component to use the core MVC).

Interesting discussion: well i think here is the answer: http://php.net/manual/en/language.oop5.decon.php

I agree with @joeforjoomla this is a real issue

@mbabker sorry but i don't agree. I'm not saying core API can't introduce a new member variable, i'm saying that because with this particular variable there is a known potential B/C break with current code it's good to add an extra check and avoit it.

@wilsonge can you please take a look at this?

Why not adding

$this->factory = new LegacyFactory; 

to your constructor. Problem solved.

I really don't see that the core application have to support any no good coding practice (because you can do it doesn't mean you should). If we add this, then we have to follow this way also with 4.0 because extensions running on 4.0 should also run on 3.9. Hard coded class names are bad for testing and we need to get better on testing to make a product quality possible our users expect.

@rdeutz i know this would simply mean problem solved but the point is that it requires an update for all extensions with this behavior.
This PR is not supposed to go into 4.0, it's perfectly fine to have such a break in 4.0 but not in 3.x

By sure it's easier to add this 3 lines PR instead of having a lot of hassle with various extensions for developers and final users.

@rdeutz

yes it can be fixed in 3rd extensions,

but this is not about easily or not easily fixing it in 3rd party extensions

neither i am worried about my own extension as i checked and i am calling parent constructor

but the problem is the big (or not big) amount of websites with non updated extensions that will hit on this

• either because developers have not updated their extensions
• or because website manager did not update for any reason, e.g. a paid extensions with and expired subscription

in this case users will blame Joomla update for breaking their website (again), and Joomla will look bad

maybe delay this PR a little closer to Joomla 3.9 RC release to force as many developers to update ?
then add it to avoid breaking websites ?

Not my call to make a final decision on this, I am giving my option, like it or hate it, mine is as valid as yours

@ggppdk finally seems to understand something.

For example 7 paid extensions of mine will result broken if not updated. Users will blame our company about a free update if the subscription is expired, we should explain that the problem is 3.9 changes backported from 4.0... and do you really think they will care about this?

They will simply complain about extensions no more working and will be forced to update to have them stil running, eventually paying again in the case of paid extensions. Not really good until 4.0

We have the same problem on the BaseModel class as well. To come here to a solution I suggest you make on both classes a private getFactory() function which returns a MVCFactoryInterface and add the NULL check then there. Then you can remove the additional check in the constructor too. Guess then we have a solution which is independent of the constructor and do not have a double check anymore.

But like it is now we have three checks which are doing basically the same.

But to be clear here, I do not support that extensions do not call the parent constructor. This is what should be fixed! My proposal is just a hackisch workaround.

I fully agree @laoneo it would be even better and more elegant code solution.

The flaw with the private getFactory() method is it still makes the protected class member variable unreliable, it not being available to subclasses, AND it stops acting as a getter but also would most likely function as a setter too in case a value is not set because of this "you're breaking B/C" argument. Code smells galore. I honestly almost feel like we would be better not having the factories in 3.9 than introducing what I feel would be hacks to get around debatably incorrect code structures to begin with.

I get where you all are coming from on the "extensions will break on the update" argument. I feel like if in my role I'm going to put weight to that argument, then I must advocate for our development strategies to change so that the most common things that break extensions on updates are only allowed on major releases:

• Introduction of new optional constructor parameters
• Introduction and use of new class member variables
• Introduction and use of new methods in existing classes (every method addition would have to be in a new class)

Am I the only one who feels like that is a terrible way to manage an API?

We can still make the variable itself private as 3.9 is not out.

If we really start adding such API restrictions, then it will be hard for further development of the core. So I'm not a fan of it.

It's better to have extensions breaking between 3.9 and 4.0 than between 3.8 and 3.9 isn't it?

it's better to have extensions use the api correctly isnt it?

The concept to use the api correctly is a concept that could be discussed for days. Because PHP allows to not call the parent constructor in a child class both ways are correct.
Again, focus on the problem and nothing else please.

There are a lot of ways to avoid this break in 3.9 despite the way that a third party extension uses the API in a way or another. We have no control over this. Our challenge is that a website won't break on a minor release update because of a 3pd extension that uses the core API in a way or another working since 3.0 to 3.8

because i can cross the road when the traffic light is red doesn't mean that i have to

The problem, any way you want to phrase it, basically boils down to the fact the class is not being instantiated in the expected state and you are arguing that a change in the constructor to add support for new features is a B/C break. The fix you are suggesting while technically valid also introduces code sniffs that suggest API design issues.

There isn't a good solution here.

@joeforjoomla: The concept to use the api correctly is a concept that could be discussed for days. Because PHP allows to not call the parent constructor in a child class both ways are correct.
Again, focus on the problem and nothing else please.

It is perfectly fine not to call the constructor but then you have to make sure that all other methods are working and overwrite this methods properly. Just changing a part of the system and blame the system that another part is not working is ignoring the real problem.

because i can cross the road when the traffic light is red doesn't mean that i have to

And sometimes you will successfully cross the road and sometimes you will be hit by a truck

This PR code has been updated following the @laoneo suggestion, using a getFactory private method but still relying on the constructor initialization. Nothing changes after all, but the problem is solved.
The same can be done for BaseDatabaseModel.

As is it's still not fixing the issue you're running into, merely bypassing it with an ugly workaround. The factory variable will still be uninitialized in your code and if you ever tried to use it you'd hit the same fatal error.

I don't see an ugly workaround, i see a more robust code. Existing code is not supposed to trigger a fatal error for using this uninitialized property, but does without this PR. If i want my extension to use ->factory i know that it must be initialized.

But you shouldn't need to handle its initialization because it is an internal detail that is taken care of unless the class isn't being instantiated in full. That's the main point. Adding constructor logic should be transparent and not require code updates and the fact that extensions require code updates to account for that added logic to me indicates other issues, which makes me stand by my earlier statement that if we really have to accept this then we must revise our development strategy so that the common things that break sites on updates (adding new methods to existing classes, adding and using new class member variables) so that those API changes are only allowed with major B/C breaking releases.

Yes, the solution is technically valid. Everyone can test this successfully all day long. That doesn't make it right.

if we really have to accept this then we must revise our development strategy so that the common things that break sites on updates (adding new methods to existing classes, adding and using new class member variables) so that those API changes are only allowed with major B/C breaking releases.

Well if you want my opinion... yes you should seriously take care and do this

https://developer.joomla.org/development-strategy.html

6.1.1 PHP
All PHP code in the /libraries folder which is not flagged as private is considered to be part of the Joomla API and subject to backwards compatibility constraints. This may be extended to include other PHP code, such as component classes, in the future.

Think for example if i had extended JControllerLegacy and added a property with the same name 'factory' but with accessibility set to private. Result: fatal error!

Then it's settled. This PR proves that there are parts of the developer community who want the API to be on full lockdown during the lifetime of a major release branch and the only way to add features is in new and independent classes because an extension might break by adding features to an established class. Makes sense to me.

By that argument, the factory feature in 3.9 must be reverted because it changes the composition of an established class.

Indeed IMO the factory feature should not have been merged in 3.9 at all. Nobody is interested in having this feature at 3.9. Everybody is interested to not break sites on update. That should be a biggest change expected for 4.0. Otherwise it would not make sense to have minor and major releases.

:eyeroll: It only breaks sites when you're overloading a constructor and not setting up the base class properties correctly.

Anywho. If you really want the WordPress strategy of every release being a "major" release, I'm cool with that. Saves us from having to have all these B/C arguments. And who needs LTS anyway?

@joeforjoomla

i have to disagree that this is a B/C break

it is clearly a bug in 3rd party components

• not initializing parent class properly that previously did not cause serious problems but it does in J3.9

this PR is about adding a workaround to avoid breaking sites,

i support that adding a workaround is really worthwhile, with a deprecation until J4.0

A deprecation isn't going to signal anything. 4.0 will come, the break will still be there, developers will still groan that we made a B/C break, and the same discussion will turn from "this is a B/C break" to "is this a needed B/C break". Because the perception I'm getting is one of "we don't want Joomla making any kind of API changes at all, anytime, ever; just fix bugs and leave everything else alone, unless fixing those bugs breaks things then don't fix those either". Might as well just shut down the repo then.

@ggppdk obviously this will be marked as deprecated and swiped out at 4.0
NO developers can groan at 4.0 if this break their extensions. 4.0 is expected as a major release requiring extensions rewriting. I've already rewritten all my extensions for 4.0 to use the new namespaced MVC and dispachter. The problem is to keep 3.x branch safe till the end with the current code.

I'm wiping my hands of this crap. If you really want to introduce this kind of philosophy steering Joomla's development, then I will find a replacement for my department coordinator role who aligns with the philosophies expressed here.

The 3.x branch is perfectly fine and does not have a compatibility break. I will openly say what the issue is. Your code is crap.

Let me put this another way. If it was discovered that allowing you to not call parent::__construct() exposed a security vulnerability would we not be able to fix that because it might break something somewhere

This is like removing stains with scissors. It is not up to the core to cater for badly written 3PD software. All points are made, fully agree with @mbabker . This kind of workarounds shoiuld not be accepted. Create a page on docs.joomla.org explaining how to fix the buggy extensions instead.

The "problem" can be solved with good communication!
Why don't we start a small section in the release notes of beta1,2,3 and rc for developers?
e.g.

Developer's corner
It is known that during the update to 3.9 some extensions might encounter a problem if they have chosen some poorly written code tactics.
Specifically test if you call the constructor etc...
If you don't it is soved like this.
Plz prepare your extension bacause Joomla does not count this a a backwards compatibility problem.
Have fun and test!

The real problem here is that the development strategy is not followed:

https://developer.joomla.org/development-strategy.html
6.1.1 PHP
All PHP code in the /libraries folder which is not flagged as private is considered to be part of the Joomla API and subject to backwards compatibility constraints. This may be extended to include other PHP code, such as component classes, in the future.

Forget about the constructor thing.

Basically here the Joomla API part that is not private is changing adding a new member variable.
The same fatal error arises if i inherit JControllerLegacy and defined a private 'factory' property.
This is not crap @mbabker, simply 2 years ago a dev can't guess if you now add a protected property with the same name to the parent class.

the factory feature in 3.9 must be reverted because it changes the composition of an established class.

@markgutton The code is B/C. The problem is that you're not using it.
SemVer explicitely allows to add new methods and properties to classes or change existing ones. It's called new features and mandates a minor version.

I'm going to close this PR as I don't see a chance to it being merged.