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

DBAL v4, Middleware, Driver, Platform, and SchemaManager implementation #61

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

GwendolenLynch
Copy link

@GwendolenLynch GwendolenLynch commented Jul 1, 2023

From DBAL 3.2 the events systems is (mostly?) deprecated in favour of middleware, and custom schema manager + factory combination.

The main purpose of this PR is to migrate as much as possible to this:

  • Jsor\Doctrine\PostGIS\Driver\Driver that extends AbstractPostgreSQLDriver
  • Jsor\Doctrine\PostGIS\Driver\Middleware that wraps the lower level Driver inside the PostGIS Driver
  • Jsor\Doctrine\PostGIS\Driver\PostGISPlatform wraps Doctrine\DBAL\Platforms\PostgreSQLPlatform and is instantiated by the PostGIS Driver
  • Jsor\Doctrine\PostGIS\Schema\SchemaManager now extends Doctrine\DBAL\Schema\PostgreSQLSchemaManager and handles schema management
  • Jsor\Doctrine\PostGIS\Schema\SchemaManagerFactory is an implementation of Doctrine\DBAL\Schema\SchemaManagerFactory
    • Creates the SchemaManager instance inside Connection
  • Functionality from Jsor\Doctrine\PostGIS\Event\DBALSchemaEventSubscriber moved to PostGISPlatform

Additionally:

  • Updated docs
  • Added @covers on most tests to get a better view of coverage
  • Updated tests and all pass
  • Updated psalm.xml and it passes

Bonus: It is now possible to extend Jsor\Doctrine\PostGIS\Types\GeographyType or Jsor\Doctrine\PostGIS\Types\GeometryType and not have the schema tool throw a tantrum. See PostGISPlatformTest

Feedback welcome, and I'm happy to add/adjust/drop relevant things.

Edit: Fixes #63


use Doctrine\DBAL\Platforms\AbstractPlatform;

final class GeoJsonType extends GeographyType
Copy link

Choose a reason for hiding this comment

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

Is this file in the wrong folder?
Would be cool to have this new type added too

Copy link
Author

Choose a reason for hiding this comment

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

No, it is meant to be a fixture with respect to this PR.

Agreed it might be a nice feature, but beyond scope of this PR 😺

Copy link

Choose a reason for hiding this comment

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

fun fact: we started using it in our own project too, but altered to be a geometry rather than a geography:

ST_GeomFromGeoJSON(%s)::geometry

Copy link
Owner

Choose a reason for hiding this comment

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

Exactly one of the reasons i only provide very basic types. Even those are probably only starting points for your own application specific types 😉

Copy link

Choose a reason for hiding this comment

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

yeah, exactly (with the added fun of having Doctrine not discovering the class if you use hypens - as type name 🤦 )

we got an "auto WSG84 converter" type, but that's a different story :-)

@thePanz
Copy link

thePanz commented Feb 20, 2024

@jsor we have been using this fork with no issues for a while now, any progress/help-needed here?

Repository owner deleted a comment from vazw Mar 4, 2024
@GwendolenLynch GwendolenLynch force-pushed the dbal-deprecations branch 3 times, most recently from 9866728 to 1b766d0 Compare March 5, 2024 13:56
@GwendolenLynch GwendolenLynch force-pushed the dbal-deprecations branch 2 times, most recently from 2bc5745 to 07e9d7a Compare March 7, 2024 16:08
@GwendolenLynch
Copy link
Author

This latest update should take care of the remaining deprecations to support DBAL v4 (and silence PHPUnit).

Tests are passing locally.

@GwendolenLynch GwendolenLynch changed the title DBAL Middleware, Driver, Platform, and SchemaManager implementation DBAL v4, Middleware, Driver, Platform, and SchemaManager implementation Mar 7, 2024
}

/** @var ColumnDiff $columnDiff */
foreach ($diff->getModifiedColumns() as $columnDiff) {
if ($columnDiff->hasChanged('srid')) {
if ($columnDiff->getOldColumn()->getPlatformOption('srid') !== $columnDiff->getNewColumn()->getPlatformOption('srid')) {
Copy link

Choose a reason for hiding this comment

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

This throws an exception when the 'srid' option is not defined (example: the column is not one from PosGis)

I would add something like this before the check:

            if( ! $columnDiff->getNewColumn()->hasPlatformOption('srid')) {
                continue;
            }

Copy link

Choose a reason for hiding this comment

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

The error is thrown when running bin/console doctrine:schema:validate -v

Copy link
Author

Choose a reason for hiding this comment

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

Thanks so much for the testing! Fix pushed.

Copy link

Choose a reason for hiding this comment

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

Thank you for the PR! 👍

Unfortunately DBAL v4 is not (yet) compatible with Symfony, thus my tests will be limited to v3 only from now on

@wesnick
Copy link

wesnick commented Mar 21, 2024

👍 This branch works for me, I was able to upgrade an existing symfony project to DBAL 4, ORM 3. Thanks!

Comment on lines 127 to 130
if(!$columnDiff->getOldColumn()->hasPlatformOption('srid') && !$columnDiff->getNewColumn()->hasPlatformOption('srid')) {
continue;
}
if ($columnDiff->getOldColumn()->getPlatformOption('srid') !== $columnDiff->getNewColumn()->getPlatformOption('srid')) {
Copy link

Choose a reason for hiding this comment

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

This check doesn't seem to work in our case.

$columnDiff->getNewColumn() has the srid platform option, but $columnDiff->getOldColumn() doesn't have it.
This makes it fail with the error Warning: Undefined array key "srid".

I don't know why $columnDiff->getOldColumn() doesn't have the srid platform option at this point, though.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the logic, does that fix it for you?

Copy link

Choose a reason for hiding this comment

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

I just tested the new logic. It doesn't throw any error any more.
But it always detects a change, which wants to update the SRID. I guess, that's because srid is not there in $columnDiff->getOldColumn(). There's probably another issue, that this doesn't get populated.

Copy link
Author

Choose a reason for hiding this comment

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

I've poked and prodded, but I can't reproduce. I hit something like this when I was first composing the PR … but now 😟

Do you have the time to generate a test case? I'll happily dig in from there!

@tasselchof
Copy link

@GwendolenLynch in the documentation you write:

Additionally, to also use the library with the Doctrine ORM, register the ORMSchemaEventListener event subscriber.

use Jsor\Doctrine\PostGIS\Event\ORMSchemaEventListener;

$entityManager->getEventManager()->addEventSubscriber(new ORMSchemaEventSubscriber());

In the example you register ORMSchemaEventSubscriber, if someone just copies it it will fail.

@tasselchof
Copy link

Also, I am getting this when I am adding ORM subscriber:

PHP Fatal error:  Uncaught Roave\PsrContainerDoctrine\Exception\DomainException: Invalid event subscriber "Jsor\Doctrine\PostGIS\Event\ORMSchemaEventListener" given, must be a dependency name, class name or an instance implementing Doctrine\Common\EventSubscriber in /../vendor/roave/psr-container-doctrine/src/Exception/DomainException.php:36

@GwendolenLynch
Copy link
Author

Thanks for your feedback, @tasselchof. I'm going to try to add some more tests to cover this.

What versions of doctrine/orm, doctrine/dbal, and doctrine/common are you using?

@tasselchof
Copy link

Thanks for your feedback, @tasselchof. I'm going to try to add some more tests to cover this.

What versions of doctrine/orm, doctrine/dbal, and doctrine/common are you using?

I've made a pull request to your branch with those fixes: GwendolenLynch#1. You can merge it to fix those and if I will find anything else - I will do another pull request. It's just one test if you wish to add it.

[app]$ composer show doctrine/*
doctrine/annotations   2.0.1  Docblock Annotations Parser
doctrine/collections   2.2.2  PHP Doctrine Collections library that adds additional functionality on top of PHP arrays.
doctrine/common        3.4.4  PHP Doctrine Common project is a library that provides additional functionality that other Doctrine projects depend on such as better reflection sup...
doctrine/data-fixtures 1.7.0  Data Fixtures for all Doctrine Object Managers
doctrine/dbal          4.0.1  Powerful PHP database abstraction layer (DBAL) with many features for database schema introspection and management.
doctrine/deprecations  1.1.3  A small layer on top of trigger_error(E_USER_DEPRECATED) or PSR-3 logging with options to disable all deprecations or selectively for packages.
doctrine/event-manager 2.0.0  The Doctrine Event Manager is a simple PHP event system that was built to be used with the various Doctrine projects.
doctrine/inflector     2.0.10 PHP Doctrine Inflector is a small library that can perform string manipulations with regard to upper/lowercase and singular/plural forms of words.
doctrine/instantiator  2.0.0  A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer         3.0.1  PHP Doctrine Lexer parser library that can be used in Top-Down, Recursive Descent Parsers.
doctrine/migrations    3.7.4  PHP Doctrine Migrations project offer additional functionality on top of the database abstraction layer (DBAL) for versioning your database schema a...
doctrine/orm           3.1.2  Object-Relational-Mapper for PHP
doctrine/persistence   3.3.2  The Doctrine Persistence project is a set of shared interfaces and functionality that the different Doctrine object mappers share.

@scripness
Copy link

Hey guys! Are there any plans on merging this or should we rollback to DBAL v3?

Copy link

@maxhelias maxhelias left a comment

Choose a reason for hiding this comment

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

A CHANGELOG.md or UPGRADE.md would be welcome to list the new classes and especially the BC of deleted or renamed class.

},
"require-dev": {
"doctrine/orm": "^2.9",
"doctrine/collections": "^2.0 || ^3.0",
"doctrine/orm": "^2.19 || ^3.0",

Choose a reason for hiding this comment

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

I think we should add a conflict like : "doctrine/orm": "<2.18" for Doctrine\ORM\Query\TokenType

@GErpeldinger
Copy link

Hello everyone, any news on this MR ? Are the changelog is the only thing missing for merging it ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doctrine deprecation
9 participants