Skip to content

Form profiler broken after 3.2.8 -> 3.3.0 upgrade #22973

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

Closed
Xymanek opened this issue May 30, 2017 · 18 comments
Closed

Form profiler broken after 3.2.8 -> 3.3.0 upgrade #22973

Xymanek opened this issue May 30, 2017 · 18 comments

Comments

@Xymanek
Copy link

Xymanek commented May 30, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no (?)
RFC? no
Symfony version 3.3.0

Happens when you open the Form panel (and only form, all other panels are ok) in the profiler and it contains any forms (No forms were submitted for this request doesn't trigger the exceptions). Clearing the cache doesn't fix it. Here is a the stack trace (very long). This didn't happen before the upgrade

Example of forms:

class RegistrationForm extends AbstractType
{
    public function buildForm (FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('network', NetworkType::class, ['label' => false])
            ->add('username', null, ['label' => false, 'attr' => ['placeholder' => 'Username']])
            ->add('email', EmailType::class, ['label' => false, 'attr' => ['placeholder' => 'Email']])
            ->add('plainPassword', RepeatedType::class, [
                'type' => PasswordType::class,
                'first_options' => ['label' => false, 'attr' => ['placeholder' => 'Password']],
                'second_options' => ['label' => false, 'attr' => ['placeholder' => 'Repeat password']],
            ])
            ->add('submit', SubmitType::class, ['label' => 'Register']);
    }

    public function configureOptions (OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => User::class,
            'validation_groups' => 'user_registration'
        ]);
    }
}
class EditUserForm extends AbstractType
{
    public function buildForm (FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('fullName')
            ->add('jobTitle')
            ->add('role', ChoiceType::class, [
                'error_bubbling' => true,
                'choices' => [
                    'Administrator' => User::ROLE_ADMIN,
                    'Developer' => User::ROLE_DEVELOPER,
                    'Watcher' => User::ROLE_READ_ONLY,
                ]
            ]);
    }

    public function configureOptions (OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => User::class,
            'csrf_protection' => false,
            'validation_groups' => 'user_add'
        ]);
    }
}
class CreateUserForm extends AbstractType
{
    public function buildForm (FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('username', TextType::class, ['error_bubbling' => true])
            ->add('email', EmailType::class, ['error_bubbling' => true]);
    }

    public function getParent ()
    {
        return EditUserForm::class;
    }
}
@dmaicher
Copy link
Contributor

dmaicher commented May 30, 2017

Are you sure its not some PHP Opcache issue?

\Symfony\Component\VarDumper\Cloner\Data now implements \ArrayAccess so this error message seems weird. => Confused things a bit... forget this comment. I can reproduce the error message if I just do {{ dump(data.foo) }} for example.

I tried to reproduce the issue on the symfony/demo app but there it works fine.

If the problem still happens can you create a fork of symfony/demo that demonstrates the problem?

@Xymanek
Copy link
Author

Xymanek commented May 30, 2017

PHP Opcache

It's disabled (as per profiler). Restarting apache (I'm using mod_php) doesn't help. I'll try upgrading another project and forking demo is the problem persists. Meanwhile here are registered bundles in case that helps:

    public function registerBundles ()
    {
        $bundles = [
            new Symfony\Bundle\FrameworkBundle\FrameworkBundle(),
            new Symfony\Bundle\SecurityBundle\SecurityBundle(),
            new Sensio\Bundle\FrameworkExtraBundle\SensioFrameworkExtraBundle(),

            new Symfony\Bundle\TwigBundle\TwigBundle(),
            new Symfony\Bundle\MonologBundle\MonologBundle(),
            new Symfony\Bundle\SwiftmailerBundle\SwiftmailerBundle(),
            new Doctrine\Bundle\DoctrineBundle\DoctrineBundle(),
            new Stof\DoctrineExtensionsBundle\StofDoctrineExtensionsBundle(),

            new JMS\SerializerBundle\JMSSerializerBundle(),
            new FOS\RestBundle\FOSRestBundle(),
            new EightPoints\Bundle\GuzzleBundle\GuzzleBundle(),

            new Oneup\FlysystemBundle\OneupFlysystemBundle(),

            new AppBundle\AppBundle(),
            new ApiBundle\ApiBundle(),
            new ServerBundle\ServerBundle(),
        ];

        if (in_array($this->getEnvironment(), ['dev', 'test'], true)) {
            $bundles[] = new Symfony\Bundle\DebugBundle\DebugBundle();
            $bundles[] = new Symfony\Bundle\WebProfilerBundle\WebProfilerBundle();
            $bundles[] = new Sensio\Bundle\DistributionBundle\SensioDistributionBundle();
            $bundles[] = new Sensio\Bundle\GeneratorBundle\SensioGeneratorBundle();
        }

        return $bundles;
    }

@Xymanek
Copy link
Author

Xymanek commented May 30, 2017

@dmaicher Just tested another project and symfony/demo (forked, cloned, composer install), exact same thing on both

@Xymanek
Copy link
Author

Xymanek commented May 30, 2017

Also rebooted the machine

@xabbuh
Copy link
Member

xabbuh commented May 30, 2017

Can you describe the steps one has to perform to reproduce your issue with the Symfony demo?

@Xymanek
Copy link
Author

Xymanek commented May 31, 2017

  1. Clone the repo
  2. composer install
  3. Go to app_dev.php/en/admin/post/new
  4. Open the form panel in profiler

That said I tested this on a new $5 droplet running Ubuntu (PHP 7.0.15) and it was working fine. The problem was observed on Windows (PHP 7.0.4). Both x64 and running Apache 2.4.18.

So I guess the problem is either the OS or the patch version...

EDIT: enabling/disabling OPcache and xdebug on either makes no difference

@chalasr
Copy link
Member

chalasr commented May 31, 2017

Works for me on OSX.

@Xymanek
Copy link
Author

Xymanek commented May 31, 2017

@chalasr What PHP version are you running?

I'll update PHP later to see if it's a problem that was patched between 7.0.4 and 7.0.15

@chalasr
Copy link
Member

chalasr commented May 31, 2017

@Xymanek 7.1.5

@xelaris
Copy link
Contributor

xelaris commented Jun 2, 2017

I was able to reproduce the issue with PHP lower than 7.0.8 and did some investigation. The root cause is a bug with serialize/unserialize in PHP, which was fixed in 7.0.8 (https://bugs.php.net/bug.php?id=72229). Due to this bug the data collected by the FormDataCollector are broken after serialize/unserialize, so they can't be used by the WebProfilerBundle to render the data. Probably the DataCollector is prone to the PHP bug since the changes made in #21638.

The issue can be reproduced using this Melody script:

<?php
<<<CONFIG
packages:
    - "symfony/form: 3.3.0"
    - "symfony/http-kernel: 3.3.0"
    - "symfony/var-dumper: 3.3.0"
CONFIG;

use Symfony\Component\Form\Forms;
use Symfony\Component\Form\Extension\DataCollector\FormDataCollector;
use Symfony\Component\Form\Extension\DataCollector\FormDataExtractor;

$form = Forms::createFormFactory()->createBuilder()->getForm();

$formDataCollector = new FormDataCollector(new FormDataExtractor());
$formDataCollector->collectConfiguration($form);
$formDataCollector->buildPreliminaryFormTree($form);

$collector = unserialize(serialize($formDataCollector));

$data = $collector->getData();

foreach ($data['forms'] as $form) {
    dump($form['id']);
}

When running with PHP >= 7.0.8 the output is "form" (as expected), otherwise null.

Status: Reviewed

@Xymanek
Copy link
Author

Xymanek commented Jun 3, 2017

@xelaris Thanks for investigating, since I didn't even get to it yet.

So what is the usual course of action in such cases? Add a note somewhere? I don't suppose it can be fixed to be used pre-7.0.8 php?

@Xymanek
Copy link
Author

Xymanek commented Jun 6, 2017

Just to confirm: updated PHP to 7.0.14 and now it works

@curry684
Copy link
Contributor

curry684 commented Jun 6, 2017

Adding a "conflicts": { "php": ">=7 <7.0.8" } to the profiler should do the job. It could cause complaints though from people not being able to upgrade and not understanding why.

Alternatively the data collector could check for itself when it is loading data with something like:

if (version_compare(PHP_VERSION, '7.0.0') >= 0 && version_compare(PHP_VERSION, '7.0.8') < 0) {
  @trigger_error("Form profiler behavior is broken in PHP 7 versions below PHP 7.0.8 due to a bug in PHP itself", E_USER_WARNING);
}

This would at least explain why it is broken as it would show up in the logs tab of the error. As the code is only run during development, and only once per submitted form, the performance impact would be negligible.

@nicolas-grekas
Copy link
Member

What about forcing FormDataCollector::$hasVarDumper to false for these PHP versions?

@curry684
Copy link
Contributor

Fine, but I'd still combine it with the warning so people know why it's happening.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 28, 2017

Note that adding a conflict won't help as that would only make composer install already released versions and prevent new ones from being installed.
And coding a workaround for 3.3 is going to be just a waste of time...
I'd propose either close as won't fix, or at best set 7.0.8 as the minimum supported PHP (ie ^5.5.9|^7.0.8) version of Symfony 3.4 (at let it be a won't fix for 3.3 really). Any preference?

@fabpot
Copy link
Member

fabpot commented Jul 28, 2017

We did bump the minimum version of PHP to a newer "patch" one in the patch when earlier PHP versions were "too" buggy. So, bumping to 7.0.8 for 3.4 is a no-brainer. I would even say that doing it for 3.3 is also a possibility (as 7.0.8 was released more than a year ago).

@curry684
Copy link
Contributor

@fabpot it would leave the same issue as @nicolas-grekas described - someone with 7.0.5 installed will then just remain stuck at 3.3.5 and wonder why it's not getting updates. When I suggested the conflicts workaround 3.3.0 was only out a week and it could've been possible to just retag 3.3.0. It wouldn't have broken anything as Composer locks on commit refs anyway.

By now the only realistic option is indeed wontfix, or the code level warning that I described. Don't think it would be too much effort to add a single warning message to the form profiler though.

fabpot added a commit that referenced this issue Jul 28, 2017
This PR was merged into the 3.3 branch.

Discussion
----------

[Form] Add notice to upgrade to PHP v7.0.8+

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22973
| License       | MIT
| Doc PR        | -

So that PHP7 < 7.0.8 users get a notice about the issue.

Commits
-------

0347e5a [Form] Add notice to upgrade to PHP v7.0.8+
@fabpot fabpot closed this as completed Jul 28, 2017
nicolas-grekas added a commit that referenced this issue Aug 3, 2017
This PR was merged into the 3.3 branch.

Discussion
----------

Bump minimal PHP version to ^5.5.9|>=7.0.8

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22973
| License       | MIT
| Doc PR        | -

As spotted in the linked issue, because of https://bugs.php.net/72229.

Commits
-------

2282a6f Bump minimal PHP version to ^5.5.9|>=7.0.8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants