Skip to content

Comments

Allow scoped and local extensions on core controllers.#908

Merged
LukeTowers merged 4 commits intodevelopfrom
scoped-local-extensions
May 29, 2023
Merged

Allow scoped and local extensions on core controllers.#908
LukeTowers merged 4 commits intodevelopfrom
scoped-local-extensions

Conversation

@mjauvin
Copy link
Member

@mjauvin mjauvin commented May 19, 2023

Complement wintercms/storm#134

@mjauvin mjauvin added enhancement PRs that implement a new feature or substantial change needs review Issues/PRs that require a review from a maintainer labels May 19, 2023
@mjauvin mjauvin added this to the v1.2.3 milestone May 19, 2023
@mjauvin mjauvin requested a review from bennothommo May 19, 2023 16:44
@mjauvin mjauvin self-assigned this May 19, 2023
@mjauvin mjauvin requested a review from LukeTowers May 19, 2023 16:45
@mjauvin mjauvin changed the title Allow Scoped and local extensions on Backend controller. Allow Scoped and local extensions on core controllers. May 19, 2023
@mjauvin
Copy link
Member Author

mjauvin commented May 19, 2023

the __call() and __callStatic() methods could also be added to ExtendableTrait, not sure if this is a good idea though.

@LukeTowers
Copy link
Member

Traits can't have magic methods AFAIK which is why they aren't there already.

@LukeTowers
Copy link
Member

Is this taking into account the existing __call & __callStatic methods on these classes / their parents?

@mjauvin
Copy link
Member Author

mjauvin commented May 19, 2023

Is this taking into account the existing __call & __callStatic methods on these classes / their parents?

It is in ExtendableTrait's extendableCall() & extendableCallStatic() methods.

@mjauvin mjauvin modified the milestones: v1.2.3, v1.2.2 May 20, 2023
@mjauvin mjauvin changed the title Allow Scoped and local extensions on core controllers. Allow scoped and local extensions on core controllers. May 20, 2023
@LukeTowers
Copy link
Member

@mjauvin I mean this PR is going to overwrite the existing base class' __call() and __callStatic() methods, we need to make sure the behavior remains the same.

@mjauvin
Copy link
Member Author

mjauvin commented May 25, 2023

@LukeTowers I adjusted CmsController & BackendController's __call() method to replicate the previous behavior.

What should we do for the the __callStatic() methods of the same two classes which previously did not exist ? Throw a badMethodCall exception like below?

    public static function __callStatic($name, $params)
    {   
        if ($name === 'extend') {
            if (empty($params[0])) {
                throw new \InvalidArgumentException('The extend() method requires a callback parameter or closure.');
            }
            self::extendableExtendCallback($params[0], $params[1] ?? false, $params[2] ?? null);
            return;
        }
        
        throw new BadMethodCallException(sprintf(
            'Method %s::%s does not exist.', static::class, $method
        ));
    }

@mjauvin
Copy link
Member Author

mjauvin commented May 25, 2023

Looking into this again, and the behavior is not changed without calling the parent's __call() / __callStatic() methods. If we don't call the ExtendableTrait's methods (extendableCall() and extendableCallStatic()), we loose the extensibility (other than the "extend()" method itself).

What do you think @bennothommo ?

@mjauvin
Copy link
Member Author

mjauvin commented May 25, 2023

The base class for CmsController & BackendController is Illuminate\Routing\Controller which has no special handling other than returning BadMethodCallException().

So behavior will not change.

Copy link
Member

@bennothommo bennothommo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this should be OK. I had to do the same thing for the database models.

@LukeTowers
Copy link
Member

Does it still throw the exact same BadMethodCall Exception if the resolution fails?

@mjauvin
Copy link
Member Author

mjauvin commented May 29, 2023

Does it still throw the exact same BadMethodCall Exception if the resolution fails?

Depends which class and whether it's static or not...

Backend\Classes\Controller throws the exact same exception.

Backend\Classes\BackendController & Cms\Classes\CmsController used to throw a php Error exception since they didn't have any __callStatic() method defined, now they will throw the same BadMethodCall exception. Also, their __call() method will now thrown the same exception as before (BadMethodException), but the error message is slightly different: "Call to undefined method %s::%s()" vs "Method %s::%s does not exist."

@LukeTowers LukeTowers added Status: Completed and removed needs review Issues/PRs that require a review from a maintainer labels May 29, 2023
@LukeTowers LukeTowers merged commit af0f866 into develop May 29, 2023
@LukeTowers LukeTowers deleted the scoped-local-extensions branch May 29, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement PRs that implement a new feature or substantial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants