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

!!! TASK: Update Doctrine Migrations to 3.0 #1880

Merged
merged 24 commits into from
Nov 13, 2020

Conversation

kdambekalns
Copy link
Member

@kdambekalns kdambekalns commented Dec 6, 2019

This updated the required version of doctrine/migrations from 1.8 to 3.0.

While there are new features in Doctrine Migrations, the reason for us to do
an upgrade is to move forward – the previously used version will not be
maintained forever… This post also gives some background on that:
https://www.doctrine-project.org/2020/04/10/doctrine-migrations-3.0.html

For a Flow user the commands remain unchanged, so far no multi-namespace
migrations are supported and the features to the "official" CLI do not matter,
since we embed the functionality in our own commands.

Breaking changes

There are three things that make this upgrade a breaking change:

  • Doctrine\DBAL\Migrations moved to Doctrine\Migrations
  • AbstractMigration changed method signatures (type delcarations added)

To adjust your PHP code (the migration files), a core migration is provided that
should fix the vast majority of existing migrations. (That core migration is in Flow
and named Version20201109224100.)

  • The "version" is the FQCN of the migration class (existing entries in the migrations table will be automatically updated)

The needed changes to the DB table where the migration status is stored are done
the first time a command that accesses that table is used. Make sure to have a current
backup and then run ./flow doctrine:migrationstatus --show-migrations. If all
went well, the migrations should all be listed as a fully-qualified class name, no
longer just a date/time string. If any errors occurred during the command, restore the
backup (the migrations table is sufficient), fix the errors and try again.

See https://github.com/doctrine/migrations/blob/3.0.x/UPGRADE.md#code-bc-breaks
and https://github.com/doctrine/migrations/blob/3.0.x/UPGRADE.md#upgrade-to-20
for a full list of other changes. Most of those are wrapped in Flow code and need no
adjustments in userland code.

Resolves #2122

@sorenmalling
Copy link
Contributor

@kdambekalns Would you like me to go over this for doctrine/orm 2.2.* ?
Did you already do any research and know of any blockers?

@kdambekalns kdambekalns force-pushed the task/update-doctrine-migrations branch from 8c07dd5 to c2eb3a3 Compare March 26, 2020 09:27
@albe
Copy link
Member

albe commented Sep 13, 2020

D'oh, now we have Doctrine Migrations 3.x (https://www.doctrine-project.org/2020/04/10/doctrine-migrations-3.0.html) - see #2026. We might also need to update doctrine/common with this #2021, though it's still supposed to be phased out. So maybe we can even just get rid of it?

@kdambekalns
Copy link
Member Author

I'll look into this (again), now that we have 7.0 coming it seems worth it! 💪

@albe
Copy link
Member

albe commented Nov 8, 2020

Can we still get this in for 7.0?

@kdambekalns
Copy link
Member Author

Good question… sorry, I didn't get around to it. @jonnitto Is the RM to ask, I guess. What are your thoughts, worth working on it today?

@kdambekalns
Copy link
Member Author

kdambekalns commented Nov 9, 2020

These command are available and need to be working (again):

  • doctrine:validate Validate the class/table mappings
  • doctrine:create Create the database schema
  • doctrine:update Update the database schema
  • doctrine:entitystatus Show the current status of entities
  • doctrine:dql Run arbitrary DQL and display
  • doctrine:migrationstatus Show the current migration status
  • doctrine:migrate Migrate the database schema
  • doctrine:migrationexecute Execute a single migration
  • doctrine:migrationversion Mark/unmark migrations as migrated
  • doctrine:migrationgenerate Generate a new migration

@kdambekalns
Copy link
Member Author

Woah, I'm at it… hold your horses. 😎

@albe
Copy link
Member

albe commented Nov 9, 2020

Just fixing the obtrusive github thingies :D sorry! I'll hold back now

@kdambekalns
Copy link
Member Author

One caveat I know of: when a property has been removed from a model, cache:flush needs to be run to avoid an error:

$ ./flow doctrine:migrationgenerate
Property Acme\Com\Domain\Model\SomeThing::$someProperty does not exist

  Type: ReflectionException
  File: Packages/Libraries/doctrine/persistence/lib/Doctrine/Persistence/Mapping/Ru
        ntimeReflectionService.php
  Line: 75

@kdambekalns kdambekalns marked this pull request as ready for review November 9, 2020 20:08
@kdambekalns kdambekalns mentioned this pull request Nov 11, 2020
This is a follow-up to 5866b3e and
raises the version on more minor - gaining support for PHP 8.
@albe
Copy link
Member

albe commented Nov 11, 2020

Regarding the psalm failures left now - guess we need to update the docblocks in our Doctrine\Query. We'll need to baseline the QueryInterface->Doctrine\Query discrepancy though.
Most other look okay, but there's also

Docblock-defined class or interface Neos\Flow\Persistence\Doctrine\Migrations\MigrationException does not exist
Docblock-defined class or interface Doctrine\DBAL\Migrations\AbortMigrationException does not exist

@kdambekalns kdambekalns force-pushed the task/update-doctrine-migrations branch from 81b3d25 to 25fc924 Compare November 11, 2020 11:31
@kdambekalns
Copy link
Member Author

Let's see what is left now…

We'll need to baseline the QueryInterface->Doctrine\Query discrepancy though.

Or we adjust that along with #2231 and relax it a bit… 🤷‍♂️

@albe
Copy link
Member

albe commented Nov 11, 2020

Yeah, we could do this. I mean at this point it's just a cosmetic change, which can happen any time.

@bwaidelich
Copy link
Member

Can someone give me an update what needs to be done here?
I'd say we should try to get it in by tomorrow or postpone it to some later release

@albe
Copy link
Member

albe commented Nov 12, 2020

AFAIS only a psalm-update-baseline (and then actually fixing things with #2231)

@kdambekalns
Copy link
Member Author

The tests should pass with a psalm baseline update (I'll do that now).

Then we have two items from #1880 (comment) and finally #1880 (comment) - these are issues that could be addressed in a follow-up PR, if you agree…

Oh, and of course we need to adjust the DB migrations in Neos and our other packages…

kdambekalns and others added 3 commits November 12, 2020 16:23
Result of running

    php -d pcre.jit=0 ~/.composer/vendor/bin/psalm \
      --config=Packages/Framework/psalm.xml --show-info=false \
      --set-baseline=Packages/Framework/psalm-baseline.xml

and throwing away a change related to the Redis ext (which I have
installed, but Travis doesn't.)
@kdambekalns kdambekalns force-pushed the task/update-doctrine-migrations branch from 811017e to 46724aa Compare November 12, 2020 15:25
@kdambekalns
Copy link
Member Author

Well… that seems to be a mismatch betweeen Psalm versions. @albe Can you check this and maybe do the baseline update for now?

@albe
Copy link
Member

albe commented Nov 12, 2020

<files psalm-version="3.18.2@19aa905f7c3c7350569999a93c40ae91ae4e1626"> yep - you need to install psalm ~3.10.0, or maybe we should finally add that as requirement to the dev-distribution

@albe albe removed the I: WIP label Nov 12, 2020
@kdambekalns
Copy link
Member Author

Thanks for updating the baseline, @albe!

yep - you need to install psalm ~3.10.0

Hm… what about something new? #2205 suggests that. Let's track this as #2243

@kdambekalns
Copy link
Member Author

I created a new issue for the known open tasks - so this could go in fast(er) for easier testing. 😬

@albe
Copy link
Member

albe commented Nov 13, 2020

Hm… what about something new?

Yeah, I have that on my radar, but I think we should then try to keep versions across our branches as close as possible (optimally in sync), because upmerging the baseline might otherwise become a hassle.

kdambekalns added a commit to kdambekalns/party that referenced this pull request Nov 13, 2020
@albe albe merged commit e5e3f48 into neos:master Nov 13, 2020
@kdambekalns kdambekalns deleted the task/update-doctrine-migrations branch November 13, 2020 15:48
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.

Update doctrine dependencies
5 participants