-
-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop PHP 7.4 and add support for psr/link
v2
#59
base: 3.0.x
Are you sure you want to change the base?
Drop PHP 7.4 and add support for psr/link
v2
#59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch looks good, but needs to target next major.
At that point, we can also use the native type declarations in parameters.
@@ -270,6 +259,7 @@ | |||
</TypeDoesNotContainType> | |||
</file> | |||
<file src="src/ResourceGenerator/ExtractCollectionTrait.php"> | |||
<MethodSignatureMismatch occurrences="1"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh-oh...
<MethodSignatureMismatch occurrences="1"> | ||
<code>protected function generateSelfLink(</code> | ||
</MethodSignatureMismatch> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh-oh ^2
@@ -53,31 +52,31 @@ public function __construct($relation, string $uri = '', bool $isTemplated = fal | |||
/** | |||
* {@inheritDoc} | |||
*/ | |||
public function getHref() | |||
public function getHref(): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is not final
: adding return types here will be a major BC break for downstream
@@ -44,7 +44,7 @@ public function getLinksByRel($rel): array | |||
/** | |||
* {@inheritDoc} | |||
*/ | |||
public function withLink(LinkInterface $link): self | |||
public function withLink(LinkInterface $link): static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a BC break for downstream consumers that implement this trait, and aren't final
class Uri | ||
use Stringable; | ||
|
||
class Uri implements Stringable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please use final
, and a lot of it @_@
@internalsystemerror cb73daf is extraneous: please rebase instead |
I'm converting this to a draft as it clearly needs more work. If we're targeting a new major, should I update all method signatures to match |
cb73daf
to
b745fba
Compare
Your pick: @weierophinney may have a stronger opinion perhaps? Supporting both would mean a new BC break release once we drop v1 (due to added signatures) |
So, the PSR evolution by-law suggests doing a new minor that targets 1.1, and which adds only a return typehint, as that's technically a non-BC break; however, the by-law misses that any class extending the implementation would experience a BC break unless they add the return typehint, as they'd be widening the return type. And, of course, as soon as you also add support for the new major of a PSR, you MUST jumpt to a new major in your implementation, due to the signature change, even if you support both versions (and we could support both 1.1 and 2.0 at that point). So, 👍 from me to bump to new major, and I think we can do |
Can't do it in 1.1 due to inherent BC breaks |
Signed-off-by: Gary Lockett <[email protected]>
Signed-off-by: Gary Lockett <[email protected]>
b745fba
to
e7128ba
Compare
Description
Best I could do to avoid BC breaks was to:
psr/link
to include^2.0
(closes Update dependency psr/link to v2 #58)psalm-baseline.xml