Make Uuid final again (aka Revert 'Remove "final" keyword to allow extending') #255

Majkl578 · 2019-01-07T15:26:54Z

Allowing inheritance of Uuid class is a terribly bad decision and encourages bad design:

There is 46 (!) public methods in Uuid class, these implicitly become public API of any descendant.
People abuse inheritance to avoid writing few lines of code without thinking of consequences.
Composition should be encouraged and even enforced, it shouldn't be _up to the consumer (opposed to " I'll agree with you that I prefer composition over inheritance, but I prefer choice even more so." by @stanlemon in Remove "final" keyword to allow extending #25).
The argumentation in [Feature] Make uuid mockable #23 is wrong, Uuid is not supposed to be mocked, it's a value object. Also there is an interface (now).

More on this topic: https://ocramius.github.io/blog/when-to-declare-classes-final/

As this is a BC break for anyone performing heretical practices, targets next major.

This reverts commit c7daee6 / PR #25.

(The build is failing now, will address that once discussed.)

* Maintain backwards compatibility with the former Uuid::isValid() and Uuid::VALID_PATTERN * Remove the assert statements in favor of str_replace and preg_match * Make the validator a featureset of the factory * Remove old suggestion about locations of constants * Remove argument count assertions * Remove VALID_PATTERN from Uuid

The Docker images currently do not have all the proper dependencies for some newly-updated dependent packages (i.e. iconv), so we're using standard Travis for the moment.

Additionally remove apigen & coveralls and lower version for doctrine/annotations.

It is not necessary anymore, because the minimal required version was already set to 7.0

- Add XSD schema, so it can be validated (and PHPStorm provides auto-completion) - Drop backupGlobals option, because false is the default value

* issues found by phpstan L0 * issues found by phpstan L1 * issues found by phpstan L2 in src/ * issues found by phpstan L2 in tests/

It helps with refactorings and static analysis, because you can easily tell if the class is missing or not.

It was removed from Travis in c479cdd

This reverts commit ad89bda.

… on Ramsey\Uuid\Converter\TimeConverterInterface::calculateTime()

…y phpstan level 3

…ast/math, as mentioned in ramsey#210

…y RandomLibAdapter::$generator not being type hinted as nullable

Majkl578 · 2019-01-07T20:26:20Z

What actual, existing problems are you solving with this massive BC break?

Can you explain on what grounds this is a massive BC break? For me it's an edge case.

ramsey · 2019-01-07T20:43:00Z

It’s a BC break for those who might be extending the class to override functionality. My understanding is that there are some who like to inject their own bits into the UUID at certain places. I think the design changes I’m proposing in #256 provide a better path for this, but it does break BC for anyone currently extending Uuid.

Ocramius · 2019-01-08T05:25:09Z

This is a BC break, but a UUID is a rigid type covered by clear specifications.

Subclassing is absolutely NOT the correct way to have alternate UUID types, and UuidInterface already provides an excellent way to implement alternate UUID variations.

The fact that the boundary was broken means that in userland you have to assume that there may be subtypes to Uuid when type-hinting against the rigid type, which per-se was already a BC break.

OskarStark · 2019-01-08T06:44:29Z

Regardless of being a BC break here or not, it can be handled and a new major could be released including final, so existing code will still work.

If I understand @Majkl578 correctly this PR is just about doing it or not (not about "don't do it, it's a BC break").
This is a ValueObject with an interface, which allow mocking very easily if needed (this is often an argument against making a class final), so this should definitely be final.

For me this is more a decision about the future and not the past, closing the API and knowing how its gonna be used is a big plus and will bring more flexibility for further development.

I am 👍

//cc @greg0ire

greg0ire

Should protected fields and methods become private?

Ocramius · 2019-01-08T07:57:54Z

Should protected fields and methods become private?

Can be done safely, plus can apply all the optimisations you want now 👍

unkind · 2019-09-09T17:48:50Z

and UuidInterface already provides an excellent way to implement alternate UUID variations.

By the way, this interface is better to be removed as well. Unnecessary abstraction.

OskarStark · 2019-09-09T19:22:36Z

Can we proceed here with a new major?

ramsey · 2019-09-09T19:27:10Z

This most definitely will be included in a new major version. I have some structural changes I intend to make and have not had time to devote to this, hence the delay.

ramsey · 2019-12-17T07:15:17Z

I just attempted to cherry-pick this commit into the new 4.x branch and realized that it won't work in its current state because DegradedUuid extends Uuid. 🤦‍♂

bradynpoulsen and others added 30 commits January 19, 2018 18:29

Update Travis builds to use Travis-installed PHP instead of Docker

ffacb03

The Docker images currently do not have all the proper dependencies for some newly-updated dependent packages (i.e. iconv), so we're using standard Travis for the moment.

Set minimum version to PHP 5.6 and use PHP 5.5 style ::class constants

1ebda49

Add GmpConverter and GmpTimeConverter

042f1d7

Update dev requirements and test suite for PHP 7+ only

5d16dbe

Additionally remove apigen & coveralls and lower version for doctrine/annotations.

Bump phpunit/phpunit development dependency to version 6.4

f6403c6

Drop paragonie/random_compat from dependencies

dd4609c

It is not necessary anymore, because the minimal required version was already set to 7.0

Add XSD schema to phpunit.xml.dist config

e52f125

- Add XSD schema, so it can be validated (and PHPStorm provides auto-completion) - Drop backupGlobals option, because false is the default value

Require phpstan and correct issues found up to level 2

d834515

* issues found by phpstan L0 * issues found by phpstan L1 * issues found by phpstan L2 in src/ * issues found by phpstan L2 in tests/

Use ::class constants where possible

361d376

It helps with refactorings and static analysis, because you can easily tell if the class is missing or not.

Ensure that RandomNodeProviderTest runs only on 64bit systems

1380db6

Change library ircmaxell/random-lib to paragonie/random-lib

cdc1e20

fix 4.x branch rebase

0709ec5

Drop remnants of HHVM from tests

ba3d525

It was removed from Travis in c479cdd

Do not use the latest goaop/framework 2.2

ad89bda

Revert "Do not use the latest goaop/framework 2.2"

e4bf54e

This reverts commit ad89bda.

Tests: configure cacheDir for AspectMock

78bb737

Do not use the latest goaop/framework 2.2

7f21db7

bumping phpstan level

b9b7369

adjusting docs to match existing tests & return types

4be1dca

bumping phpstan level

d8a1a93

adjusting docs to match existing tests & return types

904a30a

bumping phpstan level

f379939

typecasting to resolve phpstan issue triggered by return type defined…

5fafb5f

… on Ramsey\Uuid\Converter\TimeConverterInterface::calculateTime()

bumping phpstan level

e803ec4

calling unset rather than setting null to resolve issues identified b…

569f58f

…y phpstan level 3

bumping phpstan level, no issues identified

34f4904

tests pass so we can probably safely supress this error

deb3a89

bumping phpstan level, supressing error caused by docblocks on moonto…

b4fb954

…ast/math, as mentioned in ramsey#210

bumping phpstan level, swapping order of property assignment caused b…

17f954d

…y RandomLibAdapter::$generator not being type hinted as nullable

ramsey added the branch: 4.x label Jan 7, 2019

ramsey added this to the Version 4.0.0 milestone Jan 7, 2019

OskarStark approved these changes Jan 8, 2019

View reviewed changes

greg0ire reviewed Jan 8, 2019

View reviewed changes

silvadiego approved these changes Feb 3, 2019

View reviewed changes

ramsey changed the base branch from master to 4.x November 30, 2019 05:11

ramsey closed this Dec 17, 2019

ramsey reopened this Dec 17, 2019

ramsey force-pushed the 4.x branch 5 times, most recently from 21fdb2a to 6498812 Compare December 24, 2019 02:56

ramsey force-pushed the 4.x branch 4 times, most recently from 2b156cc to 1c7eba3 Compare January 9, 2020 03:55

ramsey force-pushed the 4.x branch 2 times, most recently from 8d3debe to 72a2312 Compare January 18, 2020 18:15

ramsey closed this Jan 22, 2020

Majkl578 deleted the final-again branch January 22, 2020 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Uuid final again (aka Revert 'Remove "final" keyword to allow extending') #255

Make Uuid final again (aka Revert 'Remove "final" keyword to allow extending') #255

Majkl578 commented Jan 7, 2019 •

edited

Loading

Majkl578 commented Jan 7, 2019

ramsey commented Jan 7, 2019

Ocramius commented Jan 8, 2019

OskarStark commented Jan 8, 2019

greg0ire left a comment

Ocramius commented Jan 8, 2019

unkind commented Sep 9, 2019 •

edited

Loading

OskarStark commented Sep 9, 2019

ramsey commented Sep 9, 2019

ramsey commented Dec 17, 2019

Make Uuid final again (aka Revert 'Remove "final" keyword to allow extending') #255

Make Uuid final again (aka Revert 'Remove "final" keyword to allow extending') #255

Conversation

Majkl578 commented Jan 7, 2019 • edited Loading

Majkl578 commented Jan 7, 2019

ramsey commented Jan 7, 2019

Ocramius commented Jan 8, 2019

OskarStark commented Jan 8, 2019

greg0ire left a comment

Choose a reason for hiding this comment

Ocramius commented Jan 8, 2019

unkind commented Sep 9, 2019 • edited Loading

OskarStark commented Sep 9, 2019

ramsey commented Sep 9, 2019

ramsey commented Dec 17, 2019

Majkl578 commented Jan 7, 2019 •

edited

Loading

unkind commented Sep 9, 2019 •

edited

Loading