Skip to content
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

Update to laminas/laminas-coding-standard:2.3.x, improved types and internal API #103

Merged
merged 11 commits into from Jul 6, 2022

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Jul 6, 2022

Q A
QA yes

Description

Does the donkey work upgrading to LCS 2.3

Changes to tests and src are in separate commits

…minor version

Signed-off-by: George Steel <george@net-glue.co.uk>
These errors are primarily missing class, and missing file for legacy Zend purposes

Signed-off-by: George Steel <george@net-glue.co.uk>
…che file

Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
…4 others 💪

Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel gsteel force-pushed the update-laminas-coding-standard branch from 537a567 to defeaf5 Compare July 6, 2022 15:47
@gsteel
Copy link
Member Author

gsteel commented Jul 6, 2022

@Ocramius
Integration tests are failing on 8.0 and 8.1 at

$resource = fopen($stream, $mode);

I think this is because an empty path is throwing a ValueError in fopen - In order to fix this failure, we'll need to explicitly check for the empty path and throw the expected exception right?

@gsteel
Copy link
Member Author

gsteel commented Jul 6, 2022

OK, I take that last comment back - for some reason, the error handler is being ignored on 8.0 and 8.1 - this is all stuff that hasn't been touched by this PR so I'm a little bit confused as to why CI would be failing here…

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @gsteel!

@Ocramius
Copy link
Member

Ocramius commented Jul 6, 2022

Ah, I did not notice the failures. I have another branch where I did this, and will propose a patch 🤔

@Ocramius
Copy link
Member

Ocramius commented Jul 6, 2022

Opened #104 - unsure if failures are comparable

@Ocramius Ocramius added this to the 2.13.0 milestone Jul 6, 2022
The test is checking for exceptional situations for non-stream resources and given the other cases are more likely to run (i.e. GMP is probably not installed), it's not worth the psalm errors

Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
src/Stream.php Outdated
try {
$resource = fopen($stream, $mode);
} catch (Throwable $e) {
throw new Exception\RuntimeException('Invalid stream reference provided');
Copy link
Member

Choose a reason for hiding this comment

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

We should really rethrow the previous exception

Copy link
Member

Choose a reason for hiding this comment

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

s/rethrow/include

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it's a reasonable change - tests are now passing locally for me on 8.x
… The previous behaviour was to throw what's there - I'll amend to add previous perhaps? The integration tests would fail with anything other than a RuntimeException afaict

Copy link
Member

Choose a reason for hiding this comment

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

I think this is really only in case of a stream error - an exception should be rare, but is a healthy approach to a failure.

Signed-off-by: George Steel <george@net-glue.co.uk>
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@Ocramius Ocramius self-assigned this Jul 6, 2022
@Ocramius Ocramius changed the title Update laminas coding standard Update to laminas/laminas-coding-standard:2.3.x, improved types and internal API Jul 6, 2022
@Ocramius Ocramius merged commit 739ad4d into laminas:2.13.x Jul 6, 2022
@gsteel gsteel deleted the update-laminas-coding-standard branch July 6, 2022 20:55
Ocramius added a commit to Ocramius/laminas-diactoros that referenced this pull request Jul 7, 2022
In laminas#104 (duplicate), we improved the types of internal and
public API. These were mixed with coding style improvements.

The coding style improvements have been merged separately via laminas#103,
so this patch only includes the type/docblock improvements, which should ease the potential evolution of the
API of this package.
Ocramius added a commit to Ocramius/laminas-diactoros that referenced this pull request Jul 7, 2022
In laminas#104 (duplicate), we improved the types of internal and
public API. These were mixed with coding style improvements.

The coding style improvements have been merged separately via laminas#103,
so this patch only includes the type/docblock improvements, which should ease the potential evolution of the
API of this package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants