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.

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

Fix Symfony 3 support in datagrid filter (and another bug) #3709

Merged
merged 1 commit into from
Apr 19, 2016
Merged

Fix Symfony 3 support in datagrid filter (and another bug) #3709

merged 1 commit into from
Apr 19, 2016

Conversation

wouterj
Copy link
Contributor

@wouterj wouterj commented Apr 9, 2016

No description provided.

@wouterj wouterj mentioned this pull request Apr 9, 2016
Merged
@core23
Copy link
Member

core23 commented Apr 9, 2016

Please remember, that we support symfony 2.3 too. Please leave us some more information which "other bug" are you fixing?

@wouterj
Copy link
Contributor Author

wouterj commented Apr 9, 2016

Though SonataCoreBundle, Symfony 2.3 is still supported by these changes.

Other bug is the second commit: "Fix case where showtabs if false (default)" Admin#getShowTabs() returns either a boolean (false being the default) or an array. The template currently just assumed it was an array. This means you got a big error (trying to read property X of a boolean) when the value was a boolean.

core23
core23 reviewed Apr 9, 2016
View reviewed changes
Datagrid/Datagrid.php
@@ -126,15 +126,15 @@ public function buildPager()
$this->formBuilder->add($filter->getFormName(), $type, $options);
}

$this->formBuilder->add('_sort_by', 'hidden');
$this->formBuilder->add('_sort_by', 'Symfony\Component\Form\Extension\Core\Type\HiddenType');
Copy link
Member

Choose a reason for hiding this comment

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

This only works with symfony 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't, take a look at the FormHelper and the DependencyInjectionExtension in the CoreBundle

Copy link
Member

Choose a reason for hiding this comment

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

@wouterj Stop me if I didn't understand but $this->formBuilder is a Symfony FormBuilderInterface instance.

And this instance does not accept class on SF 2.3 AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stop :) Read the docs: https://sonata-project.org/bundles/core/master/doc/reference/form_types.html#symfony3-supports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, tests proof that this is working in Symfony 2.3.

Copy link
Member

Choose a reason for hiding this comment

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

This is recent. Could I see the code part handling this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/sonata-project/SonataCoreBundle/blob/master/Form/FormHelper.php
https://github.com/sonata-project/SonataCoreBundle/blob/master/Form/Extension/DependencyInjectionExtension.php

Copy link

Choose a reason for hiding this comment

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

I couldn't make it work with symfony 2.7.11:

Could not load type "Symfony\Component\Form\Extension\Core\Type\HiddenType"

If I understand well, it would be 'hidden' instead of 'Symfony\Component\Form\Extension\Core\Type\HiddenType' and add a mapping in Sonata\AdminBundle\SonataAdminBundle for 3.x compatibility.

Copy link
Contributor

@greg0ire greg0ire Apr 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@jhcasado You understand well. @wouterj : do the test cover this part? Can either of you both please fix it ? Just like in CollectionType ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #3753

@greg0ire
Copy link
Contributor

Can you please fixup the 2 last commits into the first one ?

@wouterj
Copy link
Contributor Author

wouterj commented Apr 11, 2016

@greg0ire done

@greg0ire
Copy link
Contributor

This should be tagged as minor, and LGTM

@greg0ire greg0ire added the minor label Apr 13, 2016
@soullivaneuh soullivaneuh added patch and removed minor labels Apr 13, 2016
@soullivaneuh
Copy link
Member

@greg0ire This is just compatibility fix. It's a patch. :-)

@greg0ire
Copy link
Contributor

Indeed, my bad.

@wouterj
Copy link
Contributor Author

wouterj commented Apr 13, 2016

Well, 2.4 (and master) isn't released. I definitely think releasing 2.4 for this bundle is important. However, given that this bug existed in master, I don't think you can call master stable.

@wouterj wouterj mentioned this pull request Apr 16, 2016
Merged
4 tasks
@greg0ire
Copy link
Contributor

Well, 2.4 (and master) isn't released. I definitely think releasing 2.4 for this bundle is important. However, given that this bug existed in master, I don't think you can call master stable.

Maybe we should make a release candidate then.

@mvhirsch
Copy link
Contributor

👍

@greg0ire
Copy link
Contributor

ping @OskarStark @core23

@core23
Copy link
Member

core23 commented Apr 19, 2016

LGTM

@greg0ire greg0ire merged commit 4a8a10b into sonata-project:master Apr 19, 2016
@wouterj wouterj deleted the fix-sf-3 branch April 19, 2016 17:44
@ju1ius ju1ius mentioned this pull request Apr 21, 2016
Closed
wouterj added a commit to wouterj/SonataAdminBundle that referenced this pull request May 1, 2016
@wouterj
Revert sonata-project#3709
c7633c2
@wouterj wouterj mentioned this pull request May 1, 2016
Merged
wouterj added a commit to wouterj/SonataAdminBundle that referenced this pull request May 1, 2016
@wouterj
Use form type names instead of FQCN (reverts sonata-project#3709)
6c7e701 
My previously merged change turned out to not fix, but break the admin
panel. While there can be no garantee that this commit "fixes" anything
instead of breaking it, the change of getting a working admin panel will
be significantly increased after this PR. This because it reverts things
back to the state they were for the past 5 years (without any bug report
about it being broken).
wouterj added a commit to wouterj/SonataAdminBundle that referenced this pull request May 1, 2016
@wouterj
Use form type names instead of FQCN (reverts sonata-project#3709)
a4ded97 
My previously merged change turned out to not fix, but break the admin
panel. While there can be no garantee that this commit "fixes" anything
instead of breaking it, the change of getting a working admin panel will
be significantly increased after this PR. This because it reverts things
back to the state they were for the past 5 years (without any bug report
about it being broken).
soullivaneuh pushed a commit that referenced this pull request May 1, 2016
@wouterj @soullivaneuh
Use form type names instead of FQCN (reverts #3709) (#3753)
5009ab9 
My previously merged change turned out to not fix, but break the admin
panel. While there can be no garantee that this commit "fixes" anything
instead of breaking it, the change of getting a working admin panel will
be significantly increased after this PR. This because it reverts things
back to the state they were for the past 5 years (without any bug report
about it being broken).
@wouterj wouterj mentioned this pull request May 7, 2016
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wouterj @core23 @greg0ire @soullivaneuh @mvhirsch @jhcasado