Add a feature switch to trigger SchemaValidator in Collector #1120
Conversation
| @@ -26,9 +26,16 @@ class DoctrineDataCollector extends BaseCollector | |||
| /** @var string[] */ | |||
| private $groupedQueries; | |||
|
|
|||
| public function __construct(ManagerRegistry $registry) | |||
| /** @var bool */ | |||
| private $shouldValidateSchemaClass; | |||
There was a problem hiding this comment.
I would name it shouldValidateSchema. I don't see why there is Class in the name.
| private $shouldValidateSchemaClass; | ||
|
|
||
| /** | ||
| * @param bool $shouldValidateSchemaClass |
There was a problem hiding this comment.
please use a typehint instead, as the master branch requires PHP 7.1+
There was a problem hiding this comment.
done (I had 1.12.x in ming but @ostrolucky confirmed it's master)
bastnic
force-pushed
the
feature/feature-switch-SchemaValidator
branch
3 times, most recently
from
df47248
to
c408889
Compare
|
@stof all done. Tests added. |
|
As it's targeted a big release, should we put it to false by default? |
bastnic
force-pushed
the
feature/feature-switch-SchemaValidator
branch
2 times, most recently
from
69d6000
to
44a3011
Compare
|
Please add a summary of #1115 to the body of your commit message (but keep the |
bastnic
force-pushed
the
feature/feature-switch-SchemaValidator
branch
from
44a3011
to
9b22320
Compare
|
@ostrolucky @greg0ire all done. |
bastnic
force-pushed
the
feature/feature-switch-SchemaValidator
branch
2 times, most recently
from
e1ae6ae
to
ab82ecc
Compare
There was a problem hiding this comment.
Best reviewed with whitespace ignored https://github.com/doctrine/DoctrineBundle/pull/1120/files?utf8=%E2%9C%93&diff=unified&w=1
|
|
||
| $collector->collect(new Request(), new Response()); | ||
|
|
||
| $this->assertEmpty($collector->getEntities()); |
There was a problem hiding this comment.
Asserting this doesn't make sense to me, any particular reason to do it? Using getMappingErrors would at least make some sense
There was a problem hiding this comment.
if entities is empty, it's because we never enter in the if condition, otherwise it would be ['default' => []]. It's also the condition asked by @stof, to not display anything in the template.
You are right, I will add a test on getMappingErrors.
Calling SchemaValidator from DoctrineDataCollector can results in loading a lot of metadatas for relations of retated entities. On some profile, it can be up to 18% of the total (already heavy) pages. As there is already a command to get the schema validation, we can put this validation behind a feature switch `profiling_collect_schema_errors`. Command is: `bin console doctrine:schema:validate` Fixes doctrine#1115
bastnic
force-pushed
the
feature/feature-switch-SchemaValidator
branch
from
ab82ecc
to
1ccb822
Compare
Fixes #1115