[Form] Implemented form debugger #9082

Merged
merged 5 commits into from Sep 25, 2013
@webmozart
Symfony member

Same as #9021 (kudos to @digitalkaoz!), with some added caramel.

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

Note: Please keep in mind that this is a first version only. Not all features (especially UX/UI-wise) that I'd like to see are in there, but it's good enough for a first merge.

@fabpot
Symfony member

@bschussek Can you add some screenshots here?

@webmozart
Symfony member

Here you go :)

Toolbar:
form-debugger-1

Profiler:
form-debugger-2

@hhamon

Killer feature! Thanks for this!

@fabpot fabpot and 2 others commented on an outdated diff Sep 19, 2013
.../Component/HttpKernel/DataCollector/DataCollector.php
*/
abstract class DataCollector implements DataCollectorInterface, \Serializable
{
protected $data;
+ /**
+ * @var ValueExporterInterface
+ */
+ protected $valueExporter;
+
+ public function __construct(ValueExporterInterface $valueExporter = null)
@fabpot
Symfony member
fabpot added a note Sep 19, 2013

That's a big BC break for any DataCollector out there.

@webmozart
Symfony member

Why? It's got a default value.

@fabpot
Symfony member
fabpot added a note Sep 19, 2013

sorry, didn't see the default value.

@stof
Symfony member
stof added a note Sep 19, 2013

@bschussek But any collector out there must now call the parent constructor if they have their own constructor, otherwise the exporter is not set

@webmozart
Symfony member

@stof Check the body of the method.. ;)

@stof
Symfony member
stof added a note Sep 19, 2013

If existing collectors have a custom constructor, they need to update their code. Otherwise the body of the method is not executed.

And existing collectors are not calling parent::__construct() currently as there is no parent constructor.

@webmozart
Symfony member

Good point, I will fix this.

@webmozart
Symfony member

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ghost

OMG! +1000 ;)

@fabpot fabpot and 3 others commented on an outdated diff Sep 19, 2013
...pKernel/DataCollector/Util/ValueExporterInterface.php
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\DataCollector\Util;
+
+/**
+ * Converts PHP values into strings.
+ *
+ * @author Bernhard Schussek <bschussek@gmail.com>
+ */
+interface ValueExporterInterface
@fabpot
Symfony member
fabpot added a note Sep 19, 2013

Why do we need an interface here?

@webmozart
Symfony member

Because it's a service and other classes depend on it. Dependencies to services should always be coded against interfaces.

@fabpot
Symfony member
fabpot added a note Sep 19, 2013

For me, we need an interface only if we can have different implementations. If not, I don't see the point.

@webmozart
Symfony member

We're a framework. We should follow best practices consistently, not on a case-by-case basis, because our users will do a lot of stuff with our code that we can't foresee.

@webmozart
Symfony member

And for someone it might make sense to have an alternative implementation here.

@fabpot
Symfony member
fabpot added a note Sep 19, 2013

Which best practice are you talking about?

I think the practice here is Open/Close principle which says "software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification" (cf. wikipedia). So yes it is good to allow safe extension via interfaces.
But we should keep in mind that we want to keep a clear/simple exposure of the architecture of the components (ie. the interfaces). Interfaces lead the developer on how he should override services. IMO exposing to much is not always good. It would allow unnecessary overrides that might denote a bad practice. That is the Close part of the principle.

@nanocom
nanocom added a note Sep 19, 2013

I don't see the point of an interface here either, seems overcomplicated to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@digitalkaoz

I dont See the need for a formdatacollectorinterface. But the Rest looks great. I like the idea of the proxy type :) +1 from me

...ihatewritingonphones

@hacfi

@digitalkaoz Thanks for the hard work..I love it ;)

@Swop

Thanks @bschussek & @digitalkaoz for this feature. It will definitely be very useful.

Is there a way to somehow display constraints linked to fields in the profiler? And highlight the invalid fields which doesn't match the constraints?

@mickaelandrieu

Sounds Great !!! Available for 2.4 I hope ? :)

@pbowyer

@bschussek What would you like to achieve UX/UI-wise?

@webmozart
Symfony member

@pbowyer I'll open a new ticket for that.

@webmozart
Symfony member
@fabpot fabpot and 2 others commented on an outdated diff Sep 25, 2013
...onent/HttpKernel/DataCollector/Util/ValueExporter.php
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\DataCollector\Util;
+
+/**
+ * @author Bernhard Schussek <bschussek@gmail.com>
+ */
+class ValueExporter
+{
+ /**
+ * {@inheritdoc}
@fabpot
Symfony member
fabpot added a note Sep 25, 2013

You cannot use @inheritdoc anymore ;)

@webmozart
Symfony member

Really, why? More info about that?

@stof
Symfony member
stof added a note Sep 25, 2013

Where are you inheriting it from ? There is no parent anymore

@webmozart
Symfony member

Ooops. I thought you meant that we changed our conventions :) will fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot fabpot commented on an outdated diff Sep 25, 2013
.../Component/HttpKernel/DataCollector/DataCollector.php
- }
-
- if (null === $var) {
- return 'null';
- }
-
- if (false === $var) {
- return 'false';
- }
-
- if (true === $var) {
- return 'true';
- }
+ if (null === $this->valueExporter) {
+ $this->valueExporter = new ValueExporter();
+ }
@fabpot
Symfony member
fabpot added a note Sep 25, 2013

indent issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot fabpot commented on an outdated diff Sep 25, 2013
.../Component/HttpKernel/DataCollector/DataCollector.php
*/
abstract class DataCollector implements DataCollectorInterface, \Serializable
{
protected $data;
+ /**
+ * @var ValueExporter
+ */
+ private $valueExporter;
+
@fabpot
Symfony member
fabpot added a note Sep 25, 2013

indent issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@webmozart
Symfony member

The inheritdoc issue is fixed.

@fabpot fabpot added a commit that referenced this pull request Sep 25, 2013
@fabpot fabpot merged branch bschussek/form-debugger (PR #9082)
This PR was merged into the master branch.

Discussion
----------

[Form] Implemented form debugger

Same as #9021 (kudos to @digitalkaoz!), with some added caramel.

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

**Note:** Please keep in mind that this is a first version only. Not all features (especially UX/UI-wise) that I'd like to see are in there, but it's good enough for a first merge.

Commits
-------

89509d9 [Form] Improved form debugger
f56c577 [HttpKernel] Extracted value exporting logic of DataCollector into a separate ValueExporter class
56d78ed [Form] Decoupled methods of ResolvedFormType so that they can be overridden individually by decorators
a994a5d [Form] Merged subsriber/collector, also collect valid forms
1972a91 [Form] Added form debug collector
55de9d9
@fabpot fabpot merged commit 89509d9 into symfony:master Sep 25, 2013

1 check failed

Details default The Travis CI build could not complete due to an error
@tyx

If form config is not enabled, this line will break as it use %form.resolved_type_factory.class% parameter, defined in form.xml.

So I guess we should load it, only if form config is neabled.

edit : Here is a PR : #9137

@AlmogBaku

This is big! +1000 for this genius feature~!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment