It must be public because a JDocument instance is being set by
JControllerLegacy so the variable is accessed outside the JViewLegacy class
chain. Based on existing behavior there's no B/C break, even if it's
majorly annoying.

On Thursday, July 21, 2016, JoeforJoomla Boy notifications@github.com
wrote:

Steps to reproduce the issue

JViewLegacy has been changed adding a new reference to JDocument as public
variable:

public $document;
Expected result

This is causing a Fatal Error for any extension extending JViewLegacy with
its own class defining a $document variable as protected
Actual result

This is causing a Fatal Error. The $document reference in JViewLegacy
should at least be declared as protected as all other members variables
$_names, $_models, etc
System information (as much as possible) Additional comments

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11239, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoU6g4JfKHP3sUofUcmokeUbT08YFks5qX-xZgaJpZM4JSQs9
.

Just for cross referencing: #10839

On Thursday, July 21, 2016, JoeforJoomla Boy notifications@github.com
wrote:

Steps to reproduce the issue

JViewLegacy has been changed adding a new reference to JDocument as public
variable:

public $document;
Expected result

This is causing a Fatal Error for any extension extending JViewLegacy with
its own class defining a $document variable as protected
Actual result

This is causing a Fatal Error. The $document reference in JViewLegacy
should at least be declared as protected as all other members variables
$_names, $_models, etc
System information (as much as possible) Additional comments

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11239, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoU6g4JfKHP3sUofUcmokeUbT08YFks5qX-xZgaJpZM4JSQs9
.

Well it's a B/C break in the case that an extension like mine was extending JViewLegacy with a member variable as it was supposed to be till now:

protected $document;

So it must be avoided or existing code needs to be refactored to avoid a Fatal Error, this is the reality.

Notice that many vendors are including its own classes extending JViewLegacy, JModelLegacy, etc so this issue could have a quite large impact in B/C break for many extensions.

Based on existing behavior the variable is used in a way that it can only be declared as public or not declared at all, which is essentially the same as a public declaration. There also is not a magic setter on JViewLegacy or JObject so without implementing those there isn't a way to mark the variable as protected, which again wouldn't be the correct behavior anyway since it is accessed outside its class chain. The variable could be declared protected and set via JObject::set() but THAT would constitute a B/C break because prior to being declared the variable is for all intents and purposes public.

The only option that doesn't break code but allows it to be documented is through annotations. But that's essentially a joke as it doesn't enforce visibility the same way a code declarations do.

This is one of those sucky situations but technically correct. The existing code behavior is that the variable is used in a manner where it must be declared public because of the use in JControllerLegacy, so the only options are to either continue to not document this variable's presence or accept that documenting it breaks declarations in child classes which used a more strict visibility. It'd be the same issue if a method was added to one of these high level classes that others had declared differently in subclasses, though in the case of a different method signature instead of a fatal error it'd introduce E_STRICT issues but the same fatal would happen if there were a different visibility scope.

If it had a large impact then this would have been reported at least once
during the extensive testing period.

It has been reported now. Is not this a testing period?

The original pull request was there for a month and easily reviewable.

If we're really going to get into this argument, then honestly the only thing for Joomla to do is to go back to PHP 4 structures; no visibility scopes no problems.

Like I said, it sucks the change causes your code to throw errors. That doesn't mean your code was right previously or that the change is automatically a backward compatibility issue. I've demonstrated through practical use that an undocumented variable was being set outside of the concerned class and based on the pre-existing behavior the only options are to either continue using this undocumented variable or properly declare it based on its existing behavior, which mandates that it be declared public. At 4.0 a more restrictive visibility can be considered, until then though there is no other alternative.

According to our rules this is not a B/C break. It's basically the same case when we add a new variable and that variable name is already used by extending 3rd party classes. There is no way we can take care of that except to not add any variables, which is not a possible way for obvious reasons.

Just change your extension so your variable matches the one from the higher class (or just remove the declaration).

Closing this as it's not a bug we are going to fix for the reasons Michael already stated.