[RTM] Added a DBAL field type for UUIDs #415

aschempp · 2015-11-17T20:52:08Z

Our current UUID field types are not supported by Doctrine. I started using ORM and the binary column was not usable because it returns a resource by default.
I think Contao core bundle should provide a type to handle it's default fields.

aschempp · 2015-11-17T20:53:16Z

Btw the class is based on https://blog.vandenbrand.org/2015/06/25/creating-a-custom-doctrine-dbal-type-the-right-way/ but adjusted to our packing/unpacking. We also need to verify the SQL comment. I'm already using the type in production though and it works fine (when the field is manually added to the DB) 😎

leofeyer · 2015-11-18T07:57:13Z

Why are you adding the type in the bundle class? Shouldn't it be in a compiler pass?

aschempp · 2015-11-18T07:59:45Z

A Compiler pass is to manipulate the container. The type is not registered in the container. Same as Bootup::initAll();

aschempp · 2015-11-18T08:00:24Z

This should be for 4.2 ;-)
I'm working on more Doctrine related stuff, maybe I'll find a bridge between our models and Doctrine ORM 😎

leofeyer · 2015-11-18T08:02:25Z

Why did you omit the following method?

public function getSqlDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
{
    return sprintf('BINARY(%d) COMMENT \'(DC2Type:uuid)\'', $fieldDeclaration['length']);
}

Not that I knew what it does, it just is in the class you have linked :)

aschempp · 2015-11-18T08:04:51Z

That's what I meant with We also need to verify the SQL comment.

There is a method to tell Doctrine to add the comment, which I implemented. The getSqlDeclaration does nothing else than add the comment, but it omits a few other checks in the parent method (e.g. use BLOB if the field is too long (which should never be the case with UUID, but anyway…))

leofeyer · 2016-04-05T13:32:02Z

Is this PR still relevant? As far as I remember, we have decided against adding your Doctrine implementation.

Toflar · 2016-04-05T13:52:17Z

No matter what implementation of doctrine, the types will be needed anyway.

leofeyer · 2016-04-05T14:07:46Z

Ok, then @aschempp will you complete the PR before the release candidate?

aschempp · 2016-04-05T14:26:11Z

I'm not sure about that. We did find some potential problems with this, but if I remember correctly that was related to my Doctrine integration.

I guess everyone who want's to use Doctrine entities in their modules and has a relation to a file might need this.

leofeyer · 2016-04-05T15:06:43Z

So are you going to complete it?

leofeyer · 2016-04-28T14:26:32Z

As discussed in Mumble on April 28th, we want to add this to version 4.2. @aschempp will check if the PR is up to date and then add the RTM flag.

aschempp · 2016-05-02T16:48:20Z

Pull request updated and added full test coverage.

leofeyer · 2016-05-04T10:14:31Z

src/Doctrine/DBAL/Types/UuidArrayType.php

+            return null;
+        }
+
+        $value = @unserialize($value);


Shouldn't we be using our deserialize() logic here, which does not allow serialized objects?

https://github.com/contao/core-bundle/blob/master/src/Resources/contao/helper/functions.php#L192

As this is for data originating from the database and not for use by developers I think we can save the overhead.

The data we are running through deserialize() is also originating from the database, isn't it?

Theoretically, but deserialize is a method from the old framework which is not (necessarily) available here. Also, if you would use Doctrine serialized array storage, the same problem would be the case.

Otherwise we would need to add a secure_array type that uses the same code as our deserialize method…?

Theoretically, but deserialize is a method from the old framework which is not (necessarily) available here.

It is not available here ;)

Also, if you would use Doctrine serialized array storage, the same problem would be the case.

That's a good point.

Otherwise we would need to add a secure_array type that uses the same code as our deserialize method…?

No, it would be to specific for Contao then IMHO.

It might be available if the framework is booted. Or we could boot the framework. But we know that's somewhat wrong :D

A secure Doctrine array (one that does not allow objects) is not specific for Contao. I know Joomla had similar security issues with unserializing object (from the session though).

You mean you would replace the array type with the secure_array type? I certainly would prefer that.

No I would not replace it, because that would potentially break third-party bundles. I would just add a secure_array type that people should use in their entities. But I wonder if these things should be part of contao/core-bundle…

But I wonder if these things should be part of contao/core-bundle…

True. 😦

As discussed in Mumble on May 4th, we want to provide an secure_array type, which integrates the [https://github.com/contao/core-bundle/blob/master/src/Resources/contao/helper/functions.php#L192](deserialize check). The uuid_array type should then inherit from the secure_array type.

aschempp · 2017-01-19T15:29:22Z

I just tested ramsey/uuid-doctrine in my application, and the binary_uuid type works flawlessly. It does not support arrays though, and that's gonna be a problem whenever one needs to store a checkbox selection in a file tree.

leofeyer · 2017-01-19T15:42:15Z

It does not support arrays though

Then we should go for a custom implementation, I guess.

aschempp · 2017-01-19T16:15:34Z

Difficult to say. First of all, the core does not use Doctrine ORM, so it's difficult to say what we should support. Also, it's fairly easy to add whatever you need to your application, because this is still pretty application specific.

I just run across the issue that ramsey/uuid-doctrine is actually not using strings in PHP, but a custom Uuid object that has a __toString method. This works in most contexts, but not in all.

Regardless of what we implement, a Doctrine type that internally converts between binary and string UUID automatically is a behaviour change to the current implementation. All we actually want is blob and binary that is not converted to a resource stream… not really related to UUID actually 😂

aschempp · 2017-01-19T16:59:51Z

I have updated the PR after realising what we actually need in a real-work scenario. A UUID type that automatically converts from binary to string will not work with any current implementation, as for example searching the DB would need the binary value and not the string.

What we actually need is just a regular binary field, but it must not be stored in a resource but simply a PHP string. That's what this PR provides.

PS: yes, unit tests are missing and it needs to be rebased. But RFC ;-)

leofeyer · 2017-01-19T20:30:48Z

LGTM.

leofeyer · 2017-01-24T14:05:29Z

Can you please give me push access to this pull request so I can make the CS fixes?

leofeyer

Actually, the boot() method is supposed to remain empty (see https://insight.sensiolabs.com/what-we-analyse). Can we not register the type in the container?

leofeyer · 2017-01-24T22:18:23Z

The tests are failing with --prefer-lowest: https://travis-ci.org/contao/core-bundle/jobs/194957297

aschempp · 2017-01-25T13:01:25Z

The tests are fixed. I was actually correct, I just forgot a third place ;-)

* Make Symfony 3.2 the minimum requirement (see contao#630). * Set the encryption key from the kernel secret by default (see contao#660). * Stop using the deprecated QuestionHelper::setInputStream() method. * Update Dropzone to version 4. * Prefer the caret operator over the tilde operator in the composer.json file. * Add the contao.root_dir parameter (see contao#662). * Update the change log. * Stop using the contao-components/all meta package. * Update the README.md file. * Deprecate the contao:version command (see contao#668). * Update the installation path. * Auto-select the active page in the quick navigation/link module (see contao/core#8587). * Look up the form class and allow to choose the type (see contao/core#8527). * Add PHP Toolbox support (see contao#565). * Remove the arrow brackets in the book navigation template (see contao/core#8607). * Add a bottom padding to the buttons layer. * Add the contao.web_dir parameter (see contao/installation-bundle#40). * Fix the tests. * Match security firewall based on request scope (see contao#677). * Fix an issue found by Scrutinizer. * Use the contao.web_dir parameter in the Combiner (see contao#679). * Fix the tests. * Add stripRootDir() method to System class (see contao#683). * Add the contao.image.target_dir parameter (see contao#684). * The ContaoCoreExtension::overwriteImageTargetDir() is not deprecated. * Support custom backend routes (see contao#512). * Use the scope matcher instead of checking the request attribute (see contao#688). * Replace every occurrence of $contaoFramework with $framework. * Fix an issue found by Scrutinizer. * Fix deprecations in unit tests (see contao#687). * Added a DBAL field type for UUIDs (see contao#415). * Support importing form field options from a CSV field (see contao#444). * Fix the coding style and the unit tests. * Add the Doctrine field type in the config.yml file. doctrine: dbal: types: binary_string: class: "Contao\\CoreBundle\\Doctrine\\DBAL\\Types\\BinaryStringType" commented: true * Add a basic unit test for the BackendCsvImportController class. * Update the change log. * Fix rebuilding the search index (see contao#689). * Also handle „no origin“ and „empty origin“ in the CORS provider. * Remove an unused use statement. * Remove the security.yml file and update the README.md file. * Improve the e-mail extraction in the text element (thanks to Martin Auswöger). * Rename the Test namespace to Tests. * Update the composer.json file. * Update the .php_cs file. * Raise the minimum PHP version to 5.6 (see contao#701). * Support using objects in callback arrays (see contao#699). * Use try-finally blocks to close all output buffers when downloading a file (see contao#714). * Fix the coding style. * Only prefix an all numeric alias when standardizing (see contao#707). * Adjust the test namespaces. * Allow to manually pass a value to any widget (see contao#674). * Add a change log entry and fix the tests. * Disable the picker buttons if the main window does not show a picker. * Use the file manager instead of the file picker. * Use the site structure instead of the page picker. * Always show the selected nodes. * Add the menu builder.

leofeyer added the feature label Nov 18, 2015

leofeyer added this to the 4.1.0 milestone Nov 18, 2015

leofeyer added bug up for discussion and removed feature labels Nov 18, 2015

aschempp changed the title ~~Added a DBAL field type for UUIDs~~ [RFC] Added a DBAL field type for UUIDs Nov 18, 2015

leofeyer modified the milestones: 4.2.0, 4.1.0 Nov 18, 2015

leofeyer added feature and removed bug up for discussion labels Nov 18, 2015

leofeyer assigned aschempp Nov 18, 2015

aschempp force-pushed the feature/uuid-type branch from 1b2ad2e to a8028ed Compare December 6, 2015 20:48

leofeyer modified the milestone: 4.2.0 Apr 15, 2016

aschempp changed the title ~~[RFC] Added a DBAL field type for UUIDs~~ [RTM] Added a DBAL field type for UUIDs May 2, 2016

leofeyer reviewed May 4, 2016
View reviewed changes

leofeyer force-pushed the develop branch from d2427fe to 852dc41 Compare September 26, 2016 10:42

leofeyer force-pushed the develop branch from 1fa58ee to 001a38f Compare January 11, 2017 14:18

aschempp changed the title ~~[WIP] Added a DBAL field type for UUIDs~~ [RFC] Added a DBAL field type for UUIDs Jan 19, 2017

aschempp force-pushed the feature/uuid-type branch 2 times, most recently from d1b90e8 to 7484eca Compare January 23, 2017 21:55

aschempp changed the title ~~[RFC] Added a DBAL field type for UUIDs~~ [RTM] Added a DBAL field type for UUIDs Jan 23, 2017

leofeyer requested changes Jan 24, 2017

View reviewed changes

leofeyer added this to the 4.4.0 milestone Jan 24, 2017

aschempp force-pushed the feature/uuid-type branch from 3abc5d4 to 05b77b3 Compare January 24, 2017 20:34

aschempp added 2 commits January 24, 2017 21:46

Added binary_string DBAL type

ed4bb7c

Use a compiler pass to add the custom Doctrine type

16244de

aschempp force-pushed the feature/uuid-type branch from 05b77b3 to 16244de Compare January 24, 2017 20:46

Fixed unit tests for abstract class

4a6a221

aschempp force-pushed the feature/uuid-type branch from 067b659 to 4a6a221 Compare January 25, 2017 12:46

leofeyer added 2 commits January 27, 2017 10:27

Fix the coding style.

7cedf63

Fix the field definition in the tests.

34d1e28

leofeyer self-assigned this Jan 27, 2017

leofeyer approved these changes Jan 27, 2017

View reviewed changes

leofeyer merged commit 87b1b88 into contao:develop Jan 27, 2017

agoat mentioned this pull request Apr 10, 2017

Feature/dca picker (#46) agoat/contao-core-bundle#47

Merged

aschempp deleted the feature/uuid-type branch March 7, 2019 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTM] Added a DBAL field type for UUIDs #415

[RTM] Added a DBAL field type for UUIDs #415

aschempp commented Nov 17, 2015

aschempp commented Nov 17, 2015

leofeyer commented Nov 18, 2015

aschempp commented Nov 18, 2015

aschempp commented Nov 18, 2015

leofeyer commented Nov 18, 2015

aschempp commented Nov 18, 2015

leofeyer commented Apr 5, 2016

Toflar commented Apr 5, 2016

leofeyer commented Apr 5, 2016

aschempp commented Apr 5, 2016

leofeyer commented Apr 5, 2016

leofeyer commented Apr 28, 2016

aschempp commented May 2, 2016

leofeyer May 4, 2016

discordier May 4, 2016

leofeyer May 4, 2016

aschempp May 4, 2016

leofeyer May 4, 2016

aschempp May 4, 2016

leofeyer May 4, 2016

aschempp May 4, 2016

leofeyer May 4, 2016

leofeyer May 4, 2016

aschempp commented Jan 19, 2017

leofeyer commented Jan 19, 2017

aschempp commented Jan 19, 2017

aschempp commented Jan 19, 2017

leofeyer commented Jan 19, 2017

leofeyer commented Jan 24, 2017

leofeyer left a comment

leofeyer commented Jan 24, 2017

aschempp commented Jan 25, 2017

[RTM] Added a DBAL field type for UUIDs #415

[RTM] Added a DBAL field type for UUIDs #415

Conversation

aschempp commented Nov 17, 2015

aschempp commented Nov 17, 2015

leofeyer commented Nov 18, 2015

aschempp commented Nov 18, 2015

aschempp commented Nov 18, 2015

leofeyer commented Nov 18, 2015

aschempp commented Nov 18, 2015

leofeyer commented Apr 5, 2016

Toflar commented Apr 5, 2016

leofeyer commented Apr 5, 2016

aschempp commented Apr 5, 2016

leofeyer commented Apr 5, 2016

leofeyer commented Apr 28, 2016

aschempp commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschempp commented Jan 19, 2017

leofeyer commented Jan 19, 2017

aschempp commented Jan 19, 2017

aschempp commented Jan 19, 2017

leofeyer commented Jan 19, 2017

leofeyer commented Jan 24, 2017

leofeyer left a comment

Choose a reason for hiding this comment

leofeyer commented Jan 24, 2017

aschempp commented Jan 25, 2017