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

Prevent extension of static classes #70

Closed
wants to merge 1 commit into from
Closed

Prevent extension of static classes #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
}
},
"require": {
"php": "^5.6 || ^7.0"
"php": "^5.6 || ^7.0",
"scriptfusion/static-class": "^1"
Copy link
Member

Choose a reason for hiding this comment

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

Dependency is to be avoided for this very internal use-case

Copy link
Author

@0xPaul 0xPaul Feb 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@0xPaul 0xPaul Feb 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

  • 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. StaticClass is tagged as 1.0.0 (stable). If this is not good enough what else do you need?

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius If you're not going to respond to these points and flatly refuse to use StaticClass you can close this PR.

Copy link
Member

Choose a reason for hiding this comment

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

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. StaticClass is 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.

Copy link
Author

@0xPaul 0xPaul Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Copy it to your don't package, then.

Copy link
Member

Choose a reason for hiding this comment

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

Can do, but won't suggest roave/dont for inclusion here either

},
"require-dev": {
"athletic/athletic": "~0.1",
Expand Down
7 changes: 4 additions & 3 deletions src/ArrayUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,18 @@

namespace Zend\Stdlib;

use ScriptFUSION\StaticClass;
use Traversable;
use Zend\Stdlib\ArrayUtils\MergeRemoveKey;
use Zend\Stdlib\ArrayUtils\MergeReplaceKeyInterface;

/**
* Utility class for testing and manipulation of PHP arrays.
*
* Declared abstract, as we have no need for instantiation.
*/
abstract class ArrayUtils
final class ArrayUtils
{
use StaticClass;
Copy link
Member

Choose a reason for hiding this comment

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

Make the constructor private instead

Copy link
Author

@0xPaul 0xPaul Feb 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That's what the trait does.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Do you not think a trait called static class conveys intent clearer than an anonymous private constructor?

Copy link
Member

Choose a reason for hiding this comment

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

@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).


/**
* Compatibility Flag for ArrayUtils::filter
*/
Expand Down
5 changes: 4 additions & 1 deletion src/ErrorHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,16 @@
namespace Zend\Stdlib;

use ErrorException;
use ScriptFUSION\StaticClass;

/**
* ErrorHandler that can be used to catch internal PHP errors
* and convert to an ErrorException instance.
*/
abstract class ErrorHandler
final class ErrorHandler
{
use StaticClass;
Copy link
Member

Choose a reason for hiding this comment

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

Make the constructor private instead


/**
* Active stack
*
Expand Down
12 changes: 8 additions & 4 deletions src/Glob.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,15 @@

namespace Zend\Stdlib;

use ScriptFUSION\StaticClass;

/**
* Wrapper for glob with fallback if GLOB_BRACE is not available.
*/
abstract class Glob
final class Glob
{
use StaticClass;
Copy link
Member

Choose a reason for hiding this comment

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

Make the constructor private instead


/**#@+
* Glob constants.
*/
Expand Down Expand Up @@ -53,7 +57,7 @@ public static function glob($pattern, $flags = 0, $forceFallback = false)
* @return array
* @throws Exception\RuntimeException
*/
protected static function systemGlob($pattern, $flags)
private static function systemGlob($pattern, $flags)
{
if ($flags) {
$flagMap = [
Expand Down Expand Up @@ -94,7 +98,7 @@ protected static function systemGlob($pattern, $flags)
* @return array
* @throws Exception\RuntimeException
*/
protected static function fallbackGlob($pattern, $flags)
private static function fallbackGlob($pattern, $flags)
{
if (! $flags & self::GLOB_BRACE) {
return static::systemGlob($pattern, $flags);
Expand Down Expand Up @@ -175,7 +179,7 @@ protected static function fallbackGlob($pattern, $flags)
* @param int $flags
* @return int|null
*/
protected static function nextBraceSub($pattern, $begin, $flags)
private static function nextBraceSub($pattern, $begin, $flags)
{
$length = strlen($pattern);
$depth = 0;
Expand Down
13 changes: 7 additions & 6 deletions src/StringUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,29 +9,30 @@

namespace Zend\Stdlib;

use ScriptFUSION\StaticClass;
use Zend\Stdlib\StringWrapper\StringWrapperInterface;

/**
* Utility class for handling strings of different character encodings
* using available PHP extensions.
*
* Declared abstract, as we have no need for instantiation.
*/
abstract class StringUtils
final class StringUtils
{
use StaticClass;
Copy link
Member

Choose a reason for hiding this comment

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

Make the constructor private instead


/**
* Ordered list of registered string wrapper instances
*
* @var StringWrapperInterface[]
*/
protected static $wrapperRegistry = null;
private static $wrapperRegistry = null;

/**
* A list of known single-byte character encodings (upper-case)
*
* @var string[]
*/
protected static $singleByteEncodings = [
private static $singleByteEncodings = [
'ASCII', '7BIT', '8BIT',
'ISO-8859-1', 'ISO-8859-2', 'ISO-8859-3', 'ISO-8859-4', 'ISO-8859-5',
'ISO-8859-6', 'ISO-8859-7', 'ISO-8859-8', 'ISO-8859-9', 'ISO-8859-10',
Expand All @@ -45,7 +46,7 @@ abstract class StringUtils
*
* @var bool
**/
protected static $hasPcreUnicodeSupport = null;
private static $hasPcreUnicodeSupport = null;

/**
* Get registered wrapper classes
Expand Down