Prevent extension of static classes #70
Conversation
| "require": { | ||
| "php": "^5.6 || ^7.0" | ||
| "php": "^5.6 || ^7.0", | ||
| "scriptfusion/static-class": "^1" |
There was a problem hiding this comment.
Dependency is to be avoided for this very internal use-case
There was a problem hiding this comment.
I understand, however the dependency only does exactly what this use-case requires and prevents code duplication, and nothing more, thus I think it is appropriate. Moreover, one might opine it reads more intuitively than writing a private constructor by conveying to the reader that the class is static, without need for additional commentary or documentation.
There was a problem hiding this comment.
Yes, but that's not going to happen due to two reasons:
- it creates a dependency for all packages relying on stdlib (ref
- use-case scenario is tiny: 2LOC (vs 1LOC) per class in this package
- it is not a package in ZF, therefore you have to have a stability that is either equivalent or better than ZF's in order to include it
There was a problem hiding this comment.
- I understand this is a core package, however this is also a common pattern and it would be my intention to introduce this trait to the wider framework since the static utility class pattern is common enough to warrant this.
- I believe a private constructor is at least 3LOC if you respect PSR-2; 4LOC if you annotate that the empty constructor is intentional, as in the trait.
- I do not understand what you mean.
StaticClassis tagged as 1.0.0 (stable). If this is not good enough what else do you need?
There was a problem hiding this comment.
@Ocramius If you're not going to respond to these points and flatly refuse to use StaticClass you can close this PR.
There was a problem hiding this comment.
I understand this is a core package, however this is also a common pattern and it would be my intention to introduce this trait to the wider framework since the static utility class pattern is common enough to warrant this.
True, having a common package is useful here, and I even wrote one myself ( https://github.com/Roave/Dont ), and yes, it also uses traits.
I believe a private constructor is at least 3LOC if you respect PSR-2; 4LOC if you annotate that the empty constructor is intentional, as in the trait
Yes, that's still very tiny, and a non-functional change too. Better than a static reference to an external symbol, and an additional autoloading requirement.
I do not understand what you mean.
StaticClassis tagged as 1.0.0 (stable). If this is not good enough what else do you need?
Adding a dependency just for this, where the dependency has a stability and maintenance guarantees lower than the ones in the framework (with all good intent and faith, it's a package maintained by a single developer, and not by the framework team) is a problem.
This is especially true when the package is at the top of the hierarchy (zend-stdlib is used everywhere).
Please don't take this as mistrust, I'm just telling you that the implications are bigger than the gains, and that by a lot.
There was a problem hiding this comment.
Copy it to your don't package, then.
There was a problem hiding this comment.
Can do, but won't suggest roave/dont for inclusion here either
| abstract class ArrayUtils | ||
| final class ArrayUtils | ||
| { | ||
| use StaticClass; |
There was a problem hiding this comment.
Make the constructor private instead
There was a problem hiding this comment.
Yes, and it's an additional dependency and autoloaded trait (not to mention all the mess that traits lead to at reflection level) on a very, very core dependency.
A private constructor is more expressive than an imported trait in this case.
There was a problem hiding this comment.
Do you not think a trait called static class conveys intent clearer than an anonymous private constructor?
There was a problem hiding this comment.
@0xPaul you are indeed correct that static class conveys intent in a clearer way. What isn't clear is how the contract of the class is changed based on that trait by looking at this class.
A private constructor does convey that immediately, but indeed does not express what the meaning is (although developers usually understand that immediately).
| abstract class ErrorHandler | ||
| final class ErrorHandler | ||
| { | ||
| use StaticClass; |
There was a problem hiding this comment.
Make the constructor private instead
| abstract class Glob | ||
| final class Glob | ||
| { | ||
| use StaticClass; |
There was a problem hiding this comment.
Make the constructor private instead
| abstract class StringUtils | ||
| final class StringUtils | ||
| { | ||
| use StaticClass; |
There was a problem hiding this comment.
Make the constructor private instead
|
Loggerheads. |
|
@0xPaul please don't be like that: I really am just doing my job as gatekeeper :| |
|
Good job. |
4 participants
Merely declaring a class
abstractdoes not correctly convey intent nor prevent misuse. By marking the classfinalwe prevent extension and by using theStaticClasstrait we prevent instantiation.