I'm not sure if this has unwanted side effects.

FWIW, I've been running this patch on one of my testing sites for a week now and haven't noticed any issues. That's not to say that there aren't any edge cases though.

There's a part of me that agrees that this is a pointless behavior, and there's a part of me that hesitates to accept this into 3.x at this point considering aside from the 3.9 feature release we really should be focusing on branch stability and not introducing potentially problematic API changes this late in the game. In theory all this does is remove a pointless restriction that someone tried to create, but we do need to be careful that this doesn't come back to bite someone in the backside.

If you're fairly confident that this PR won't break anything, it could always be merged and released with 3.9. Any latent failure due to oversight can be remedied with a patch release if such a failure were to occur.

I personally highly doubt that enabling extra functionality would adversely affect any extension. Those extensions with functionality dependent on AdminController would seem to already have redirects or other facilities in place to skirt by the change.

If it is preferred, I can have a couple colleagues look over this PR to test it further in production to see if we notice any additional effects among extensions that aren't in my test environment. Just let me know your preferred course of action and whether you would like supporting testing evidence.

whether you would like supporting testing evidence

Testing and feedback is always good.

Personally I'm just erring on the side of caution with this one. I really don't think it's actually going to cause an issue, but stranger things happen with the most innocent of changes.

The only thing I can think of is that someone extends the AdminController class and calls parent->display for whatever reason. Then the controller instantiating is different, because BaseController->display will be called and this display method is doing much more than returning $this. It might be only a very small chance that something is going wrong, but I wouldn't merge it in 3.x.

While I recognize that there is the potential for a bug-fix burden on some developers, I don't believe this oddly-specific code smell is worth the justification for holding back the PR until 4.x.

IMO, developers with bad practices of obtaining an indirect internal reference to $this through parent::display() without the expectation of side-effects shouldn't necessarily be rewarded with compatibility while simultaneously holding back further improvement of the CMS.

I was able to test this successfully without issue. It is a little surprising to find out that AdminController doesn't support display views by default. In my opinion, it seems more realistic that most people using this class would not be relying on display() returning $this.

@jeremyvii please mark your Test as successfully:

• open Issue Tracker
• Login with your github-Account
• Click on blue "Test this"-Button above Authors-Picture
• mark your Test as successfully
• hit "submit test result"

@clayfreeman

I have not found any issues in our extension,
but you can not predict what with happen with a NNN 3rd party extension,
also there might be some smaller or bigger security issue with some extension

why don't you just declare display task in you derived controller
and in it just call the grandparent display task ?

then if this is change is made in J4 your could also add an IF there to call the parent display task for J4

why don't you just declare display task in you derived controller
and in it just call the grandparent display task ?

Without Reflection PHP doesn't have a concept of "call the grandparent display task", meaning you either copy the grandparent method code into your class or you use Reflection to do something that you weren't intended to do in the first place.

Without Reflection PHP doesn't have a concept of ...

hhmm, is there a known problem if one uses
get_parent_class()
?

	public function display($cachable = false, $urlparams = false)
	{
		return get_parent_class(get_parent_class($this))::display($cachable, $urlparams);
	}

[EDIT]

		return get_parent_class(get_parent_class(get_class()))::display($cachable, $urlparams);

this is better alternative, it will work even when you have derived classes,

get_parent_class(get_parent_class($this)))::display()
will work only if you do not have more derived classes,

because in the case that $this is an object of the derived class ... you will have a loop

That works if you're accessing a static method, in which case you should just use the class name of the class whose static method you want to call in the first place.

You get a class name from get_parent_class(). With your code, you are invoking Joomla\CMS\MVC\Controller\BaseController::display() in a static context, meaning you're going to create errors because $this won't be defined.

Again, PHP does not have the concept of direct access to a grandparent method. If it did, we would have a grandparent keyword. You might get lucky and through inheritance $this->foo() can access a method two levels up, but that's just class inheritance 101.

I really do not know since which PHP version this works correctly
it may have since PHP 4 or some PHP 5.x

i know it is weird, but

somename::somemethod()

is a static call
only if somename is not found in the inheritage change chain of the class ancestors
if found it works as a regular method call

thus $this is bound correctly and visibility rules (like private, protected) are also respected

i printed $this just now to confirm, the correct object is printed (my derived class object)

Turn on E_STRICT reporting and you're going to have problems. That is not a supported PHP behavior and should not be relied on at all in anything except a 3v4l.org demonstration.

I have tested this item ✅ successfully on 1f2cd4b

I have tested this item ✅ successfully on 1f2cd4b

@ggppdk You're correct in saying that you can use the scope resolution operator to explicitly specify which method in the inheritance chain is used. This is my current workaround in one of my controllers that extends AdminController:

  /**
   * Allow this controller to be used with a view.
   *
   * This method overrides the default implementation to circumvent the
   * restriction placed on the `AdminController::display()` method by Joomla!
   *
   * @param   bool            $cachable   Whether the result of this method
   *                                      should be cached (if enabled).
   * @param   array           $urlparams  An array of sanitized URL parameters.
   *
   * @return  BaseController              `$this` to support method chaining.
   */
  public function display($cachable = FALSE, $urlparams = []) {
    // Override the inherited display method to /actually/ do something
    return BaseController::display($cachable, $urlparams);
  }

While it's completely possible for me to include this workaround in all children of AdminController, the point of this PR is that I shouldn't have to do so.

@mbabker Please see the following:

ini_set('display_errors', '1');
error_reporting(~0); // enables all errors as `~0 === 0xFFFF...`
class Foo {public function hello(){echo "hi\n"; return $this;}}
class Bar extends Foo {public function hello(){return $this;}}
class FooBar extends Bar {public function hello(){return Foo::hello();}}
$fb = new FooBar();
var_export($fb->hello());
error_reporting(E_ALL | E_STRICT);
var_export($fb->hello());

You can safely use the scope resolution operator with no repercussions in latest PHP.

@ggppdk I'd also like to state that given the name of the method, if a developer is expecting no side-effects from parent::display(), then security should be the least of their concerns.

    return BaseController::display($cachable, $urlparams);

I was going to offer this as an alternative,
but then wondered if you have are using the non-namespaced names

also i thought something safer that does not hard code grandparent class name without real need,

although of course we know these class names will never change in during the lifetime of major version, it would be a big B/C break if they changed

I'd also like to state that given the name of the method, if a developer is expecting no side-effects from parent::display(), then security should be the least of their concerns.

it is not important if you are right or wrong
if you have read some angry comments in the past here about things broken by a change that was not so much needed, then you would understand why people managing this repository are cautious about a change that they can not predict if it will have side-effects,

myself i was just a little worried of this change for our extension, although i did not find something

I have tested this item ✅ successfully on 1f2cd4b

As stated on this page:

We have a promise that a Joomla 3 extension will run on Joomla 4 and a namespaced Joomla 4 extension on Joomla 3.9.

In the pursuit of keeping this promise, I argue that this PR should be accepted for the 3.x branch if it is considered acceptable for the 4.x branch. If not in 3.9, then 3.10. This change is not necessary, no, but is recommended to provide functionality that should have been available for AdminController all along.

If this PR is rejected in favor of only applying these changes to the 4.x branch, then Joomla's original promise will be broken in this instance. I hope this can be avoided, as I would like to reduce the number of bodges that are required to have cross-version extension support.

@mbabker Should I close this request in favor of delaying it until 3.10?

Made up my mind. Let's just merge this and roll with it (ensuring the change is documented). The FormController does not impose this same restriction and there is really no logical reason to have it. If it does prove to be problematic this should be reverted and applied to 4.0 only.