[Form] Fix DocumentType #193

dbu · 2015-03-28T19:10:56Z

while working on symfony-cmf/blog-bundle#59 and hunting down the last test failure, i found that the phpcr_document form type did not work but throw exceptions.

i also noticed that there is no test for this form type, neither functional nor unit.

dbu · 2015-03-28T19:15:53Z

Form/Type/DocumentType.php

+        $loader = function (Options $options) use ($type) {
+            $qb = $options['query_builder'] ?: $options['em']->createQueryBuilder()->fromDocument($options['class'], 'a');
+
+            return $type->getLoader($options['em'], $qb, $options['class']);


this is essentially what every version of symfony since 2.3 is doing, so should not be needed to overwrite this.

okay, i solved that mistery. @guilhermeblanco fixed this 10 day ago! symfony/symfony@529d99c - that commit is not yet in a 2.3 release.

should we still do this to work with older versions?

but it will get into 2.3? if we do not duplicate the method which versions of Symfony2 will not be supported? if we do keep this method, we should add a note about when we can remove the method.

it is now in 2.3.27 - but either we would need to up our minimal requirement to that symfony version (and also somehow avoid 2.6 < 2.6.6) or we could see if we can do a version check at this point and explode if the version is not new enough. or only overwrite if we are on a too old version of symfony. not sure what is best and the most elegant?

imho increasing the min version or setting conflicts are the way to go. less code to maintain for us.

dbu · 2015-04-06T11:54:05Z

composer.json

@@ -40,7 +40,8 @@
        "burgov/key-value-form-bundle": "to edit assoc multivalue properties. require version 1.0.*"
    },
    "conflict": {
-        "phpcr/phpcr-shell": "<1.0.0-beta1"
+        "phpcr/phpcr-shell": "<1.0.0-beta1",
+        "symfony/framework-bundle": ">=2.6.0,<2.6.6"


is this the correct way to specify we conflict with 2.6 versions < 2.6.6 but 2.3.27 is ok?

what about 2.4 and 2.5? should we start to conflict with those?

did 2.4 and 2.5 get the fix? I assume not?

in this case maybe just do the following in the require: ~2.3.27,~2.6

dbu · 2015-04-06T11:55:51Z

updated. what should we do about the query builder? just hope it does not matter for now and go with the direct find?

dbu · 2015-04-07T08:33:49Z

composer.json

@@ -18,7 +18,7 @@
    "minimum-stability": "dev",
    "require": {
        "php": ">=5.3.3",
-        "symfony/framework-bundle": "~2.3",
+        "symfony/framework-bundle": "~2.3.27 || ~2.6.6 || ~2.7",


we drop compatibility with 2.4 and 2.5 as they are not fixed.
i also updated the .travis.yml to not attempt to build those versions.

btw, , means AND, || means OR.

but why do we need 2.3.27+ / 2.6.6+ ?

ah .. because of the fixes to the DoctrineType I guess

https://github.com/symfony/symfony/pull/13997/files

lsmith77 · 2015-04-09T15:07:40Z

the build failure looks strange .. otherwise +1 to merge

dbu · 2015-04-09T16:57:29Z

i now allow 2.7 to fail - has nothing to do with this pull request - i just started adding 2.7.

i now added a functional test - however, that test does not fail even with older versions of symfony. it seems, so i am not sure i got things right.

dbu · 2015-04-10T17:02:13Z

hm, and even when 2.6.6 is used, the blog thing still fails: https://travis-ci.org/symfony-cmf/BlogBundle/jobs/56075966

will need to investigate some more to figure out what we do differently there than in the test here.

lsmith77 · 2015-05-01T15:23:17Z

so merge this?

dbu · 2015-05-01T15:30:47Z

i never managed to make my tests fail. however, the BlogBundle still always fails, even with a recent enough version. there must be something i miss.

lets wait with merging until i figured out what is happening, maybe we do not need the restrictions on symfony versions as they are proposed here.

psyray · 2015-05-17T18:38:02Z

@duanesteel find a solution for our Sylius ContentBundle problem and it seems this involve the getEntitiesByIds method in the PhpcrOdmQueryBuilderLoader class (Form/ChoiceList/PhpcrOdmQueryBuilderLoader.php)
Look at his post

Is this a proper fix or a workaround ?

dbu · 2015-05-20T07:35:27Z

ok, i dug through this and its not exactly nice.

symfony older than 2.6.6 did not build a loader when no query_builder option was supplied but ended up calling $entities = $this->em->getRepository($this->class)->findAll(); in the EntityChoiceList. so things worked fine when all options where are loaded. since 2.6.6, a default query builder is created and the loader instantiated, triggering the issue all the time.

the other issue is EntityChoiceList::getChoicesForValues that tried to do a query $this->em->getRepository($this->class)->findBy(array($this->idField => $values)); when there is no loader and it has not already loaded objects. but this does not work with phpcr-odm. this is also fixed in 2.6.6

i looked into workarounds for the older symfony versions, but the way the choiceList is built in DoctrineType is extremely inconvenient to customize.

so i think we need to bump the minimal version to 2.6.6

dbu · 2015-05-20T08:03:30Z

ok, i think i got it right now. including actually respecting the query builder if there is one provided.

@psyray may i ask you to check one more time if this works as expected now?

psyray · 2015-05-20T10:22:04Z

The fix-phpcr_document-formtype branch ?

dbu · 2015-05-20T10:36:25Z

@psyray yep, this branch. i changed what is in this fix to respect the query builder if its supplied.

psyray · 2015-05-20T11:08:05Z

Seems to work in Sylius with Symfony 2.3.27

lsmith77 · 2015-05-20T14:13:20Z

we still have build failures. 5.3 needs more memory and there is some regression with 2.7

dbu · 2015-05-20T15:27:48Z

yeah 2.7 seems to render a less nice html for forms. i made the test assumptions more flexible.

for the lowest deps, i try to remove the memory limit. problem is that composer on 5.3 consumes a lot more memory than on other platforms so when there are many options of versions to compare it explodes. if it does not work, we could try 5.3 instead of 5.3.3 or stop building on 5.3

@lsmith77 do you think this can be released as bugfix release once merged?

dbu · 2015-05-20T16:29:41Z

.travis.yml

      env: SYMFONY_VERSION=2.7.* SYMFONY_DEPRECATIONS_HELPER=weak

 before_install:
  - composer self-update || true
+  - echo 'memory_limit = -1' >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini


this needs to be conditional, does not work for the hhvm build

…that fixed DoctrineType

lsmith77 · 2015-05-20T20:06:20Z

@psyray can you test #201 ? this is on top of the 1.2 branch so that we could tag it as 1.2.4 as master isn't ready yet to be released.

psyray · 2015-05-20T21:31:01Z

The fix_form_type branch, with the #201 commit inside, is tested in latest Sylius master version on Symfony 2.3.27.
Everything is fine 👍

Tested in my own project too which is in 2.6.6 👍

lsmith77 · 2015-05-22T10:14:15Z

@psyray 1.2.4 has been released -> https://github.com/doctrine/DoctrinePHPCRBundle/releases/tag/1.2.4

psyray · 2015-05-22T15:19:47Z

great, thanks for the job 👍

jeffsacco · 2015-05-23T13:08:41Z

I am new to this who thing. I updated my composer.json on doctrine/phpcr-odm to 1.2.4 as shown in the screenshot and then ran composer update and got this error. I am sure I am doing something wrong here. Could someone walk me through the process to get this update? Many thanks in advance.

lsmith77 added the in progress label Mar 28, 2015

dbu reviewed Mar 28, 2015
View reviewed changes

dbu mentioned this pull request Mar 29, 2015

Version upgrades symfony-cmf/blog-bundle#59

Closed

dbu force-pushed the fix-phpcr_document-formtype branch 2 times, most recently from 08bcad6 to 1035776 Compare April 6, 2015 11:53

dbu reviewed Apr 6, 2015
View reviewed changes

dbu force-pushed the fix-phpcr_document-formtype branch 2 times, most recently from 28a69b1 to ab5032f Compare April 7, 2015 07:29

dbu changed the title ~~[WIP] fix the phpcr_document form type~~ Require symfony versions that fixed the bug in the form type DoctrineType Apr 7, 2015

dbu force-pushed the fix-phpcr_document-formtype branch from ab5032f to 5373efc Compare April 7, 2015 08:33

dbu reviewed Apr 7, 2015
View reviewed changes

dbu force-pushed the fix-phpcr_document-formtype branch from 5373efc to 073c728 Compare April 9, 2015 16:55

dbu force-pushed the fix-phpcr_document-formtype branch from 073c728 to 0dc969b Compare April 10, 2015 17:01

dbu mentioned this pull request May 10, 2015

[ContentBundle] Static page saving is broken Sylius/Sylius#2696

Closed

lsmith77 added the review label May 11, 2015

dbu force-pushed the fix-phpcr_document-formtype branch from 0dc969b to 5373efc Compare May 18, 2015 06:23

dbu force-pushed the fix-phpcr_document-formtype branch from 5373efc to a3ca595 Compare May 20, 2015 08:01

psyray mentioned this pull request May 20, 2015

Important notice about Symfony version, maintainers please read ASAP Sylius/Sylius#2754

Closed

dbu changed the title ~~Require symfony versions that fixed the bug in the form type DoctrineType~~ [Form] Fix DocumentType May 20, 2015

dbu force-pushed the fix-phpcr_document-formtype branch 2 times, most recently from 2786126 to 832dc84 Compare May 20, 2015 15:26

dbu reviewed May 20, 2015
View reviewed changes

[Forms] Fix PhpcrOdmQueryBuilderLoader and depend on symfony version …

7782dda

…that fixed DoctrineType

lsmith77 force-pushed the fix-phpcr_document-formtype branch from 832dc84 to 7782dda Compare May 20, 2015 16:32

lsmith77 closed this May 21, 2015

lsmith77 removed the review label May 21, 2015

This was referenced May 21, 2015

[Forms] Fix PhpcrOdmQueryBuilderLoader and depend on symfony version that fixed DoctrineType #201

Merged

Merge 1.2 #202

Merged

lsmith77 deleted the fix-phpcr_document-formtype branch May 22, 2015 10:14

psyray mentioned this pull request Aug 21, 2015

Can't create pages/routes/blocks/menu in demo Sylius/Sylius#3139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Fix DocumentType #193

[Form] Fix DocumentType #193

dbu commented Mar 28, 2015

dbu Mar 28, 2015

dbu Mar 29, 2015

lsmith77 Apr 3, 2015

dbu Apr 5, 2015

lsmith77 Apr 5, 2015

dbu Apr 6, 2015

lsmith77 Apr 6, 2015

dbu commented Apr 6, 2015

dbu Apr 7, 2015

lsmith77 Apr 9, 2015

lsmith77 May 21, 2015

lsmith77 May 22, 2015

lsmith77 May 22, 2015

lsmith77 commented Apr 9, 2015

dbu commented Apr 9, 2015

dbu commented Apr 10, 2015

lsmith77 commented May 1, 2015

dbu commented May 1, 2015

psyray commented May 17, 2015

dbu commented May 20, 2015

dbu commented May 20, 2015

psyray commented May 20, 2015

dbu commented May 20, 2015

psyray commented May 20, 2015

lsmith77 commented May 20, 2015

dbu commented May 20, 2015

dbu May 20, 2015

lsmith77 commented May 20, 2015

psyray commented May 20, 2015

lsmith77 commented May 22, 2015

psyray commented May 22, 2015

jeffsacco commented May 23, 2015

[Form] Fix DocumentType #193

[Form] Fix DocumentType #193

Conversation

dbu commented Mar 28, 2015

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

dbu commented Apr 6, 2015

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

lsmith77 commented Apr 9, 2015

dbu commented Apr 9, 2015

dbu commented Apr 10, 2015

lsmith77 commented May 1, 2015

dbu commented May 1, 2015

psyray commented May 17, 2015

dbu commented May 20, 2015

dbu commented May 20, 2015

psyray commented May 20, 2015

dbu commented May 20, 2015

psyray commented May 20, 2015

lsmith77 commented May 20, 2015

dbu commented May 20, 2015

Choose a reason for hiding this comment

lsmith77 commented May 20, 2015

psyray commented May 20, 2015

lsmith77 commented May 22, 2015

psyray commented May 22, 2015

jeffsacco commented May 23, 2015