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

B/C break for change in JViewLegacy #11239

Closed
joeforjoomla opened this Issue Jul 21, 2016 · 9 comments

Comments

Projects
None yet
4 participants
@joeforjoomla
Copy link
Contributor

joeforjoomla commented Jul 21, 2016

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

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Jul 21, 2016

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
.

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Jul 21, 2016

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
.

@joeforjoomla

This comment has been minimized.

Copy link
Contributor

joeforjoomla commented Jul 21, 2016

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.

fatal_error

@joeforjoomla

This comment has been minimized.

Copy link
Contributor

joeforjoomla commented Jul 21, 2016

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.

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Jul 21, 2016

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.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Jul 21, 2016

@joeforjoomla

This comment has been minimized.

Copy link
Contributor

joeforjoomla commented Jul 21, 2016

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

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Jul 21, 2016

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.

@Bakual

This comment has been minimized.

Copy link
Contributor

Bakual commented Jul 22, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment