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 Composer lock file and fix test failures #70

Closed
wants to merge 8 commits into from

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Mar 1, 2021

Q A
QA yes

Description

Updates the composer lock file with i18n-mvc and flash messenger packages.

  • Tests should still fail on lowest because of the namespace rename from Laminas\Mvc\Router to Laminas\Router
  • The UrlIntegrationTest should still fail because the request retrieved from the container is not a ConsoleRequest

@gsteel
Copy link
Member Author

gsteel commented Mar 1, 2021

The UrlIntegrationTest is problematic in that it is unclear to me why a legacy console request is expected. I believe that in the past we may have been able to guarantee a console request was present. Given that Console itself is deprecated, can this test be left in a flaky state, given that the expected output is still the same? I would imagine that the test will be deleted in the not too distant future anyhow… Paging @Ocramius for review :)

@gsteel gsteel changed the title Updates composer lock to include I18N and Flash Messenger dependencies Update Composer lock file and fix test failures Mar 1, 2021
@Ocramius
Copy link
Member

Ocramius commented Mar 1, 2021

it is unclear to me why a legacy console request is expected

Probably for testing purposes - the test is most likely there to ensure BC compatibility.

Given that Console itself is deprecated, can this test be left in a flaky state, given that the expected output is still the same?

Sadly not: deprecation is not removal of support.

I would imagine that the test will be deleted in the not too distant future anyhow…

On the 3.0.0 release, we can certainly remove it, but in 2.x, it must stay in place if it was ever green in the past.

@@ -89,10 +86,6 @@ protected function setUp(): void
{
$cwd = __DIR__;

$this->routeMatchType = class_exists(V2RouteMatch::class)
Copy link
Member

Choose a reason for hiding this comment

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

This conditional indicates that we can deal with V2 or V3. Can we somehow reduce that via composer.json restrictions (support only one version)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made the minimum version of Laminas\Navigation ^2.8.1 in dev which is the version where support was added for Laminas\Router vs Laminas\Mvc\Router.

Copy link
Member

Choose a reason for hiding this comment

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

Does "laminas/laminas-router": "^3.0.1", cover that then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed that if --dev will only install v3, then it's simply not possible to test against v2 without changing the dev requirement of router to 2||3 or something like that - so 2.8.1 of navigation fixes the test failures and renders the checks for v2 router obsolete???

Copy link
Member Author

Choose a reason for hiding this comment

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

P.S. I think that the dev requirement of navigation could actually be bumped 2.9.x due to 7.3 support getting added around that release

Copy link
Member

Choose a reason for hiding this comment

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

We can add a "conflict": {"laminas/laminas-router": "<3.0.1"} to solve that

test/Helper/UrlIntegrationTest.php Show resolved Hide resolved
test/Helper/UrlTest.php Show resolved Hide resolved
@Ocramius Ocramius added the Bug Something isn't working label Mar 1, 2021
@Ocramius Ocramius modified the milestones: 2.13.0, 2.12.1 Mar 1, 2021
Ocramius added a commit that referenced this pull request Mar 1, 2021
@Ocramius
Copy link
Member

Ocramius commented Mar 2, 2021

This was manually merged, thanks @gsteel!

@Ocramius Ocramius closed this Mar 2, 2021
@gsteel gsteel deleted the invalid-composer-lock-file branch July 8, 2021 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants