search

laminas / laminas-diagnostics

A set of components for performing diagnostic tests in PHP applications
https://docs.laminas.dev/laminas-diagnostics/
BSD 3-Clause "New" or "Revised" License
55 stars 32 forks source link

Support for `doctrine/migrations:^3` #10

Closed cgrabenstein closed 3 years ago

cgrabenstein commented 4 years ago
Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA no

Description

I tried to tackle issue #7 and add support for doctrine/migrations v3. This PR introduces the DependencyFactory which is used to access the number of all migrations and the number of the executed ones.

This change however is not compatible with v2, that's why I have marked it as a draft. I have no experience in how to support two different versions of a dependency and need some help here.

If someone could show me a suitable way how to add support for v3 without breaking v2, I'll gladly update the code and finalize it by adding the required tests.

weierophinney commented 4 years ago

I suggest bumping the minimum supported PHP version to at least 7.2 as part of this patch (if you look at the test matrix, 5.6 just fails entirely, and there's no reason to support unsupported PHP versions at this time).

One note - in the --prefer-lowest test jobs, we get a failure around the mock object passed to the DoctrineMigration constructor (see https://travis-ci.com/github/laminas/laminas-diagnostics/jobs/391297630#L2024); you may need to bump the minimum supported PHPUnit version to mitigate that.

weierophinney commented 4 years ago

One note - in the --prefer-lowest test jobs, we get a failure around the mock object passed to the DoctrineMigration constructor (see https://travis-ci.com/github/laminas/laminas-diagnostics/jobs/391297630#L2024); you may need to bump the minimum supported PHPUnit version to mitigate that.

And now I understand the request for help in your PR description, as this is due to the Doctrine Migrations version.

You have a couple of options.

One, remove the typehint, and check for one of the two types in the constructor, raising an exception for something that is neigher.

Two, introduce a different class for v3 of Doctrine Migrations, so users can switch between the versions based on what they have installed.

cgrabenstein commented 4 years ago

Thanks for your input @weierophinney. Is there an option that you prefer? I'm good with both, although I slightly tend to option one, using one class, without the constructor typehint. It will avoid the confusion that will occur when having a DoctrineMigration and another DoctrineMigrationV3 (?) class.

I will update the draft accordingly. Let me know, should you prefer having two separate classes.

weierophinney commented 4 years ago

As a maintainer, I tend to prefer stricter typing and less logical branching, but since this is an end-user facing class, and the original has no version in the name, I think option one will be less confusing for the user. Let's go with that!

cgrabenstein commented 3 years ago

I finally had the time to finish the PR. Travis checks were not triggered, so I can't tell, if it works the way I implemented it. Help is appreciated.

weierophinney commented 3 years ago

@cgrabenstein Please review the Travis build - there are a number of failing jobs, likely due to differences in which Doctrine Migrations version is being installed: https://travis-ci.com/github/laminas/laminas-diagnostics/builds/212201132

I'd suggest adding some code to the various test cases that are testing against specific migrations versions to skip if that version is not currently installed; as an example:

if (! $someLogicToDetermineWhichVersionIsInstalled) {
    $this->markTestSkipped('Skipping as doctrine/migrations VERSION is not installed');
}

(We were running into Travis hour limits in November, which is why there were no build results flagging the problem for you; I made a change to a constructor and pushed it just now, in part to trigger a build, which is why we can now see build results.)

cgrabenstein commented 3 years ago

Great! Will check it later, thanks for the heads up.

cgrabenstein commented 3 years ago

Ok, so I finally got the tests and everything running. Two issues remaining:

So, any help is really appreciated (ping @weierophinney)

weierophinney commented 3 years ago

My suggestion is that we merge this post- #15, and release with 1.7.0; that way, we don't even have to worry about 5.6.

I'm putting it on my to-do; hopefully I can get to it this week.

Ocramius commented 3 years ago

15 released - re-targeting this for 1.8.0, but will need a rebase

cgrabenstein commented 3 years ago

@Ocramius Did the rebase. Pipeline's green also, finally! Sorry for the messy commits, if you prefer I can squash them together.

Ocramius commented 3 years ago

@cgrabenstein ok, I did a review (didn't see this one before, sorry if it feels like I'm butting in).

Overall, I feel like dropping ^1 support is acceptable and will give us better test suite stability here.

cgrabenstein commented 3 years ago

@Ocramius no worries, thanks for the review. I resolved your points and dropped support for ^1