-
Notifications
You must be signed in to change notification settings - Fork 203
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
EZP-26885: Refactor Field Types external storages to inject gateways and connection handler via DI #1985
EZP-26885: Refactor Field Types external storages to inject gateways and connection handler via DI #1985
Conversation
95cb22c
to
424b4a2
Compare
ca2da9e
to
a674bff
Compare
Example of actual implementation of DoctrineStorage gateway is available in the PR #1993. |
review ping @andrerom @kore @Nattfarinn |
+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, but some inline nitpicks to fix
doc/upgrade/6.10.md
Outdated
@@ -0,0 +1,166 @@ | |||
# Upgrade steps from 6.9 to 6.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6.10 to 6.11 by now
doc/upgrade/6.10.md
Outdated
Abstract base class `eZ\Publish\Core\FieldType\StorageGateway` for the gateway of a Field Type external storage and the | ||
`eZ\Publish\Core\FieldType\StorageGateway::setConnection` method are deprecated. | ||
|
||
Extend the `eZ\Publish\SPI\FieldType\StorageGateway` class and inject Database Handler directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to better signal that the change is not something field types have to do immediately, start the sentence by saying something like:
To prepare your FieldType for future storage engines and avoid using the deprecated abstract, extend the ..
doc/upgrade/6.10.md
Outdated
Abstract base class `eZ\Publish\Core\FieldType\GatewayBasedStorage` for Field Type external storage | ||
which uses gateway is deprecated. | ||
|
||
Use `eZ\Publish\SPI\FieldType\GatewayBasedStorage` instead and inject external |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar comment here
'%s extends deprecated %s. Extend %s instead', | ||
static::class, | ||
self::class, | ||
\eZ\Publish\SPI\FieldType\GatewayBasedStorage::class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
place in use section to not use FQCN inline
@@ -15,6 +15,8 @@ | |||
/** | |||
* Storage gateway base class to be used by FieldType storages. | |||
* | |||
* @deprecated Since 6.10. Use {@link \eZ\Publish\SPI\FieldType\GatewayBasedStorage} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6.11, same everywhere else in this PR
Another thing, this deprecation should also be mentioned with a point in doc/bc/6.11 with reference to the upgrade doc for further info. |
a674bff
to
cec4b56
Compare
doc/bc/changes-6.11.md
Outdated
## Changes | ||
|
||
- EZP-26885: Field Type external storage | ||
- Field Type external storage gets directly injected with a proper external storage gateway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is misleading isn't it, people will need to migrate as stated in upgrade doc in order for this change to affect them. As in if they stay on old abstract nothing will change or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I wasn't sure how to indicate this. Maybe I just should keep ## Changes
section empty and keep ## Deprecations
only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think so
we almost always first deprecate, and in a later release change or remove ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, all docs in 13a9dc2
cec4b56
to
13a9dc2
Compare
## Deprecations | ||
|
||
- EZP-26885: Field Type external storage | ||
- Abstract base class `eZ\Publish\Core\FieldType\StorageGateway` for the gateway of a Field Type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you also list up eZ\Publish\Core\FieldType\GatewayBasedStorage
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry :( Somehow I thought we have only two deprecations in Core. I looked over entire diff and in fact we have 4 in total. Added as a fixup in 23722a4 for easier review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amended in a6d83a7
23722a4
to
a6d83a7
Compare
Great work @alongosz, merging :) You can now rebase the other one when you have the time ;) |
Part 1 of EZP-26885 implementation.
This PR is the first step towards solving EZP-26885. To be able easily inject Doctrine instead of Legacy DatabaseHandler we need to make the latter fully injectable. Up to now, external storage gateway was chosen by context passed by
\eZ\Publish\Core\Persistence\Legacy\Content\StorageHandler
.This PR refactors Field Type-specific external storages so they get injected with proper, FT-specific gateways.
Each gateway gets injected with
\eZ\Publish\Core\Persistence\Doctrine\ConnectionHandler
.In the future Legacy gateway injected into FT-specific external storage can be replaced with Doctrine gateway which would use
\Doctrine\DBAL\Connection
.TODO:
GatewayBasedStorage
which refactored FT storages should extend.StorageGateway
to be extended by refactored FT ext. storages gateways.\eZ\Publish\Core\FieldType\GatewayBasedStorage
which sets and gets gateways from context.\eZ\Publish\Core\FieldType\StorageGateway
which allows setting connection manually, without DI.Keyword
FT external storage.BinaryFile
andMedia
FT external storage.Image
FT external storage.MapLocation
FT external storage.User
FT external storage.Page
FT external storage.RichText
FT external storage.Url
FT external storage.