Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Allow overriding the class skeleton #32

Merged
merged 7 commits into from
Dec 11, 2017
Merged

Allow overriding the class skeleton #32

merged 7 commits into from
Dec 11, 2017

Conversation

geerteltink
Copy link
Member

This adds the ability to override the CLASS_SKELETON constant. Ideally you create your own command and pass your custom class skeleton in the process method.

Build on top of #31.

@@ -55,9 +56,13 @@ public function process($class, $projectRoot = null)
$content = str_replace(
['%namespace%', '%class%'],
[$namespace, $class],
self::CLASS_SKELETON
$classSkeleton !== null ? $classSkeleton : self::CLASS_SKELETON
Copy link
Member

Choose a reason for hiding this comment

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

$classSkeleton ?: self::CLASS_SKELETON

should be enough here.

* @return string
* @throws CreateMiddlewareException
*/
public function process($class, $projectRoot = null)
public function process($class, $projectRoot = null, $classSkeleton = null)
Copy link
Member

Choose a reason for hiding this comment

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

As we change signature here it is BC Break... Isn't it?

Copy link
Member Author

@geerteltink geerteltink Jun 22, 2017

Choose a reason for hiding this comment

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

No it's not. Only an optional parameter is added. It should not break anything.

Copy link
Member

Choose a reason for hiding this comment

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

Depends of PHP version we have got warning or strict standards error.

https://3v4l.org/ONv3N

class A {
    public function x($p = null)
    {
    }
}

class B extends A {
    public function x() {
        return __CLASS__;
    }
}

$b = new B;
var_dump($b->x());
Output for 7.0.0 - 7.2.0alpha2
Warning: Declaration of B::x() should be compatible with A::x($p = NULL) in /in/ONv3N on line 15
string(1) "B"
Output for 5.6.0 - 5.6.30
Strict Standards: Declaration of B::x() should be compatible with A::x($p = NULL) in /in/ONv3N on line 15
string(1) "B"

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with that. But it's not really useful to extend the class since its only use would be to add a custom class skeleton. But since it's a constant you can't overwrite that one. It will keep getting the constant from the original class, unless you override the process method, in which case this doesn't apply.

Copy link
Member

Choose a reason for hiding this comment

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

You can override the constant, but you have to use there static::CONST instead of self::CONST.

class A {
    const MYCONST = 'A';

    public function x() {
        return static::MYCONST;
    }
}

class B extends A {
    const MYCONST = 'B';
}

$b = new B;
var_dump($b->x());

https://3v4l.org/9tmtn

Output for 5.6.0 - 5.6.30, hhvm-3.15.4 - 3.19.0, 7.0.0 - 7.2.0alpha2
string(1) "B"

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, but self::CONST is used in this class. It's the reason I had to copy the entire class and add my own code.

Anyway, I think it's easier to let it override the class skeleton by injecting it in the process method from a custom command. If you extend the CreateMiddleware class you need to create a custom command anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but if you change self:: to static:: you don't need to change signature so you are not creating possible BC Break.

Copy link
Member Author

Choose a reason for hiding this comment

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

This package is still not released as stable, so we'll create version 0.5.0. Problem solved :D

On a serious note: What I'm trying to say is if you change self:: to static:: you need to extend the CreateMiddleware class and you need to write a custom command. If you inject the class skeleton you can reuse the original CreateMiddleware class for multiple purposes and only need to create a custom commands.

In my case I have 3 templates. api, html (HtmlResponse) and normal Middleware. So if I can inject the class skeleton I have to write only 3 command classes. Otherwise I need to extend the CreateMiddleware class 3 times and write 3 command classes.

Copy link
Member

Choose a reason for hiding this comment

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

0.5.0 sounds good then, thanks!

This PR adds detection if a class already exists. If it does it will
throw an exception and won't overwrite the existing middleware.
This adds the ability to override the CLASS_SKELETON constant.
Ideally you create your own command and pass your custom class
skeleton in the process method.
@@ -55,9 +56,13 @@ public function process($class, $projectRoot = null)
$content = str_replace(
['%namespace%', '%class%'],
[$namespace, $class],
self::CLASS_SKELETON
$classSkeleton ?: self::CLASS_SKELETON
Copy link
Member

Choose a reason for hiding this comment

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

We can remove it at all !
We can set default value for the param to self::CLASS_SKELETON:

public function process($class, $projectRoot = null, $classSkeleton = self::CLASS_SKELETON)

Copy link
Member Author

Choose a reason for hiding this comment

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

Even better.

Copy link
Member Author

Choose a reason for hiding this comment

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

or static:: ? :D

Copy link
Member

Choose a reason for hiding this comment

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

self::, for static:: you will get:

Fatal error: "static::" is not allowed in compile-time constants

@@ -41,10 +41,11 @@ public function process(ServerRequestInterface $request, DelegateInterface $dele
/**
* @param string $class
* @param string|null $projectRoot
* @param string|null $classSkeleton
Copy link
Member

Choose a reason for hiding this comment

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

Now we shouldn't have here null.

@geerteltink
Copy link
Member Author

Some examples I'm currently using: https://gist.github.com/xtreamwayz/9a2515a72fa8af717b6ff2acaabd7adc

I think we can take this even a step further and change the constant references form self:: to static::. That way only the original CreateMiddlewareCommand needs to be extended with a CLASS_SKELETON constant. No need to copy the configure and execute methods.

@weierophinney weierophinney merged commit 96a74f4 into zendframework:master Dec 11, 2017
weierophinney added a commit that referenced this pull request Dec 11, 2017
weierophinney added a commit that referenced this pull request Dec 11, 2017
weierophinney added a commit that referenced this pull request Dec 11, 2017
weierophinney added a commit that referenced this pull request Dec 11, 2017
@weierophinney
Copy link
Member

Thanks, @xtreamwayz!

@geerteltink geerteltink deleted the feature/allow-override-skeleton branch December 12, 2017 08:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants