Skip to content

[DoctrineBridge] Add support for doctrin/dbal v2.6 types #22689

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 12, 2017
Merged

[DoctrineBridge] Add support for doctrin/dbal v2.6 types #22689

merged 1 commit into from
Jul 12, 2017

Conversation

jvasseur
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Add support for the following doctrine types in the property info extractor:

  • date_immutable
  • datetime_immutable
  • datetimetz_immutable
  • time_immutable
  • dateinterval

I didn't include the json type since it can be anything that can be stored in a json.

And add support for the dateinterval type for the form type guesser (the form component doesn't support using DateTimeImmutable instances).

@jvasseur jvasseur changed the title Add support for doctrin/dbal v2.6 types [DoctrineBridge] Add support for doctrin/dbal v2.6 types May 11, 2017
@fabpot fabpot added this to the 3.4 milestone May 11, 2017
@Hanmac
Copy link
Contributor

Hanmac commented May 12, 2017

i just wonder if the new types should add more DBALType:: constants too?

@jvasseur
Copy link
Contributor Author

@Hanmac we can't use the constants for the new types if we want to stay compatible with doctrine/dbal < 2.6 where they don't exists.

@Hanmac
Copy link
Contributor

Hanmac commented May 12, 2017

@jvasseur ah i thought the constants where in our part too.
then i am okay with that.

we might use the constants when we drop the support for the older versions.

are the DBAL Type objects itself useful for this? i think no, but i am not sure

PS: in the Guesser should 'Symfony\Component\Form\Extension\Core\Type\CollectionType' be replaced with CollectionType::class ?

@jvasseur
Copy link
Contributor Author

The Type objects are not used by the guesser/property extractor, the decision is only done based on the name of the type.

I'm 👎 for using ::class in the Guesser. It would mean that if we do that for all the types (only possible after 2.8 end of support) the class will have a lot of use at the start of the file just for this.

@stof
Copy link
Member

stof commented May 15, 2017

The Form component supports reading immutable DateTime instances

@jvasseur
Copy link
Contributor Author

@stof but it can't write back DateTimeImmutable instances meaning that you would have to do the conversion from a DateTime instance into a DateTimeImmutable in your setter.

I prefer to wait for proper support for DateTimeImmutable in the form component to add it to the guesser.

@jvasseur jvasseur changed the base branch from master to 3.4 May 18, 2017 09:05
@jvasseur jvasseur force-pushed the doctrine-26-types branch from ff5257c to 0328a59 Compare May 19, 2017 10:54
@jvasseur jvasseur force-pushed the doctrine-26-types branch from 0328a59 to b30fd4f Compare June 19, 2017 18:45
xabbuh
xabbuh reviewed Jul 4, 2017
View reviewed changes
src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php
@@ -117,6 +117,15 @@ public function getTypes($class, $property, array $context = array())
case DBALType::TIME:
return array(new Type(Type::BUILTIN_TYPE_OBJECT, $nullable, 'DateTime'));

case 'date_immutable':
case 'datetime_immutable':
case 'datetimetz_immutable':
Copy link
Member

Choose a reason for hiding this comment

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

We don't have tests covering these three types, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the time type was tested (and not the date, datetime and datetimetz) so I decided to do the same for the immutable variants.

nicolas-grekas
nicolas-grekas approved these changes Jul 12, 2017
View reviewed changes
src/Symfony/Bridge/Doctrine/CHANGELOG.md
@@ -1,6 +1,10 @@
CHANGELOG
=========

3.4.0
-----
* added support for doctrine/dbal v2.6 types
Copy link
Member

Choose a reason for hiding this comment

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

missing blank line just before

@nicolas-grekas
Copy link
Member

Thank you @jvasseur.

@nicolas-grekas nicolas-grekas merged commit b30fd4f into symfony:3.4 Jul 12, 2017
nicolas-grekas added a commit that referenced this pull request Jul 12, 2017
@nicolas-grekas
feature #22689 [DoctrineBridge] Add support for doctrin/dbal v2.6 typ…
3b2f9fb 
…es (jvasseur)

This PR was merged into the 3.4 branch.

Discussion
----------

[DoctrineBridge] Add support for doctrin/dbal v2.6 types

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Add support for the following doctrine types in the property info extractor:
 - date_immutable
 - datetime_immutable
 - datetimetz_immutable
 - time_immutable
 - dateinterval

I didn't include the json type since it can be anything that can be stored in a json.

And add support for the dateinterval type for the form type guesser (the form component doesn't support using DateTimeImmutable instances).

Commits
-------

b30fd4f Add support for doctrin/dbal 2.6 types
@jvasseur jvasseur deleted the doctrine-26-types branch July 12, 2017 12:10
@javiereguiluz javiereguiluz mentioned this pull request Jul 16, 2017
Closed
@HeahDude HeahDude mentioned this pull request Jul 31, 2017
Closed
This was referenced Oct 18, 2017
Merged
Merged
@Guite Guite mentioned this pull request Dec 13, 2017
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh xabbuh approved these changes

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
DoctrineBridge Feature Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
7 participants
@jvasseur @Hanmac @stof @nicolas-grekas @fabpot @xabbuh @carsonbot