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

Symfony multi-version compatibility #60

Merged
merged 4 commits into from Jun 26, 2016

Conversation

jmclean
Copy link
Contributor

@jmclean jmclean commented Jun 5, 2016

Fixed polycollection in SF 2.8+. Prototypes are now indexed by getBlockPrefix() as intended.

Update checkbox grid for new choice lists. Choice lists were completely redone in SF 2.7 and then DoctrineChoiceLoader changed in 3.1.

Tested in SF 2.3, 2.7, 2.8, 3.0, and 3.1. See https://github.com/infinite-networks/form-demo/

* Fix polycollection in SF 2.8+
* Update checkbox grid for new choice lists
* Tested in SF 2.3, 2.7, 2.8, 3.0, and 3.1
@King2500
Copy link
Contributor

King2500 commented Jun 5, 2016

Looks good.

I'll test this in our project (we're using PolyCollectionType) later.

@King2500
Copy link
Contributor

King2500 commented Jun 5, 2016

Sorry, i'm getting Exception when i send the form:

image

Full Stack Trace

[1] InvalidArgumentException: Unable to determine the Type for given data
    at n/a
        in F:\HtDocs\myproject\vendor\infinite-networks\form-bundle\Form\EventListener\ResizePolyFormListener.php line 126

    at Infinite\FormBundle\Form\EventListener\ResizePolyFormListener->getTypeForData(array('content' => array('text' => 'asdfasdf', 'format' => 'bbcode', 'visible' => '1'), 'position' => '2', 'page' => '1', '_type' => 'My\AppBundle\Form\Type\ArticleTextSegmentFormType', '_title' => 'Text'))
        in F:\HtDocs\myproject\vendor\infinite-networks\form-bundle\Form\EventListener\ResizePolyFormListener.php line 242

    at Infinite\FormBundle\Form\EventListener\ResizePolyFormListener->preSubmit(object(FormEvent), 'form.pre_bind', object(EventDispatcher))
        in  line 

    at call_user_func(array(object(ResizePolyFormListener), 'preSubmit'), object(FormEvent), 'form.pre_bind', object(EventDispatcher))
        in F:\HtDocs\myproject\app\cache\dev\classes.php line 1858

    at Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(array(array(object(BindRequestListener), 'preBind'), array(object(TrimListener), 'preSubmit'), array(object(CsrfValidationListener), 'preSubmit'), array(object(ResizePolyFormListener), 'preSubmit')), 'form.pre_bind', object(FormEvent))
        in F:\HtDocs\myproject\app\cache\dev\classes.php line 1773

    at Symfony\Component\EventDispatcher\EventDispatcher->dispatch('form.pre_bind', object(FormEvent))
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\EventDispatcher\ImmutableEventDispatcher.php line 43

    at Symfony\Component\EventDispatcher\ImmutableEventDispatcher->dispatch('form.pre_bind', object(FormEvent))
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\Form\Form.php line 546

    at Symfony\Component\Form\Form->submit(array(array('content' => array('text' => 'asdf', 'format' => 'bbcode', 'visible' => '1'), 'position' => '1', 'page' => '1', '_type' => 'My\AppBundle\Form\Type\ArticleTextSegmentFormType', '_title' => 'Text'), array('content' => array('text' => 'asdfasdf', 'format' => 'bbcode', 'visible' => '1'), 'position' => '2', 'page' => '1', '_type' => 'My\AppBundle\Form\Type\ArticleTextSegmentFormType', '_title' => 'Text'), array('content' => array('url' => 'http://asdfasdfasdf/', 'visible' => '1'), 'position' => '3', 'page' => '1', '_type' => 'My\AppBundle\Form\Type\ArticleVideoSegmentFormType', '_title' => 'Video')), true)
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\Form\Form.php line 567

    at Symfony\Component\Form\Form->submit(array('mainCategory' => '2', 'title' => 'adsfasdfasdf', 'segments' => array(array('content' => array('text' => 'asdf', 'format' => 'bbcode', 'visible' => '1'), 'position' => '1', 'page' => '1', '_type' => 'My\AppBundle\Form\Type\ArticleTextSegmentFormType', '_title' => 'Text'), array('content' => array('text' => 'asdfasdf', 'format' => 'bbcode', 'visible' => '1'), 'position' => '2', 'page' => '1', '_type' => 'My\AppBundle\Form\Type\ArticleTextSegmentFormType', '_title' => 'Text'), array('content' => array('url' => 'http://asdfasdfasdf/', 'visible' => '1'), 'position' => '3', 'page' => '1', '_type' => 'My\AppBundle\Form\Type\ArticleVideoSegmentFormType', '_title' => 'Video')), 'date' => array('date' => '2016-06-05', 'time' => '21:29'), 'marker' => '', 'special' => '', 'author' => '1', '_token' => 'fip1lqqrXLXnf9ngB5R61z6W_zQgJ-miC4aqXRv5rn0'), true)
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationRequestHandler.php line 116

    at Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationRequestHandler->handleRequest(object(Form), object(Request))
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\Form\Form.php line 489

    at Symfony\Component\Form\Form->handleRequest(object(Request))
        in F:\HtDocs\myproject\src\My\AppBundle\Controller\Admin\ArticleController.php line 86

    at My\AppBundle\Controller\Admin\ArticleController->editAction(object(Request), 'news', null)
        in  line 

    at call_user_func_array(array(object(ArticleController), 'editAction'), array(object(Request), 'news', null))
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\HttpKernel.php line 139

    at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), '1')
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\HttpKernel.php line 62

    at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), '1', true)
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel.php line 69

    at Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel->handle(object(Request), '1', true)
        in F:\HtDocs\myproject\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\Kernel.php line 185

    at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
        in F:\HtDocs\myproject\web\app_dev.php line 33

My FormType:

class ArticleFormType extends AbstractType
{
    /**
     * {@inheritdoc}
     */
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            // amoung other fields here ...
            ->add('segments', PolyCollectionType::class, [
                'label' => 'Inhalt',
                'types' => [
                    ArticleTextSegmentFormType::class,
                    ArticleVideoSegmentFormType::class,
                ],
                'allow_add' => true,
                'allow_delete' => true,
                'prototype' => true,
                'prototype_data' => [
                    'my_article_text_segment' => new ArticleTextSegment($article, $prototypeTextContent),
                    'my_article_video_segment' => new ArticleVideoSegment($article, $prototypeVideoContent),
                ],
            ])
            //  ...
    }
}

Tested with your compat repo @ Symfony 2.8.6

@jmclean
Copy link
Contributor Author

jmclean commented Jun 5, 2016

Looks like you have the _type set to the FQCN of the model class but I expected it to be set to the value of getBlockPrefix (see form-demo).

I'll look for a way to keep BC here. In the meantime does your project work if you use getBlockPrefix there? Both with and without this PR?

@King2500
Copy link
Contributor

King2500 commented Jun 5, 2016

No, using the block prefix didn't work.

And starting from Symfony 2.8 block names as types are deprecated. That's why we changed this in recent InfiniteFormBundle update. PolyCollectionType works similar to the standard Symfony CollectionType. In Symfony 2.8+ you should also use FQCN for the type option. (types option in PolyCollection is analogous to that).

protoype_data keys should also be FQCN.

Neither block names nor FQCN worked with your pull-request.

@jmclean
Copy link
Contributor Author

jmclean commented Jun 5, 2016

The _type field is the hidden field in each row that identifies what kind of row it is. It doesn't have to represent an actual form type and we could have called it _key instead. In principle, its value is arbitrary.

How would you feel if I made it truly arbitrary? Then you could keep the FQCN or choose any short string to identify it. It's used as the key of the prototypes array so it makes it easier to render the prototypes in Twig. (Also I just don't like putting FQCNs in my HTML. :) )

I'll see what I can do about prototype_data later.

@merk
Copy link
Contributor

merk commented Jun 6, 2016

I'd like to see it remain arbitrary, not everyone will want to expose the
underlying type FQCNs in their HTML source.
On 6 Jun 2016 9:40 am, "Jonathan McLean" notifications@github.com wrote:

The _type field is the hidden field in each row that identifies what kind
of row it is. It doesn't have to represent an actual form type and we could
have called it _key instead. In principle, its value is arbitrary.

How would you feel if I made it truly arbitrary? Then you could keep the
FQCN or choose any short string to identify it. It's used as the key of the
prototypes array so it makes it easier to render the prototypes in Twig.
(Also I just don't like putting FQCNs in my HTML. :) )

I'll see what I can do about prototype_data later.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#60 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAQ-UU6UNvkqXRD-GJ2XIoXMiMlqnDPbks5qI15qgaJpZM4IuTk2
.

@jmclean
Copy link
Contributor Author

jmclean commented Jun 6, 2016

OK, new commit added. You can now use any string there.

I had a look for the prototype_data option in the InfiniteFormBundle and it doesn't seem to exist as far as I can tell. Do you have a custom form extension that implements it?

* Fix the polycollection type_name test
* Tweak the checkbox grid test for better code coverage
@Sydney-o9
Copy link

Hi guys, is there a reason why this is not merged?

@King2500
Copy link
Contributor

I'll test it again.

@jmclean
Copy link
Contributor Author

jmclean commented Jun 23, 2016

I was planning to update the docs before merging but I got sidetracked. I'll get it done in the next few days if there are no objections.

@jmclean jmclean merged commit 93730b0 into infinite-networks:master Jun 26, 2016
@jmclean
Copy link
Contributor Author

jmclean commented Jun 26, 2016

There! Docs updated, PR merged, and BC maintained.

@King2500
Copy link
Contributor

Looks good. No more errors in our project with the new code. 👍

@Sydney-o9
Copy link

That's awesome - amazing job @jmclean

@jmclean jmclean mentioned this pull request Jul 17, 2016
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.

None yet

4 participants