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

Add a feature switch to trigger SchemaValidator in Collector #1120

Merged
merged 1 commit into from Jan 7, 2020
Merged

Add a feature switch to trigger SchemaValidator in Collector #1120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 25 additions & 18 deletions DataCollector/DoctrineDataCollector.php
View file
Open in desktop
Expand Up @@ -26,9 +26,13 @@ class DoctrineDataCollector extends BaseCollector
/** @var string[] */
private $groupedQueries;

public function __construct(ManagerRegistry $registry)
/** @var bool */
private $shouldValidateSchema;

public function __construct(ManagerRegistry $registry, bool $shouldValidateSchema = true)
{
$this->registry = $registry;
$this->registry = $registry;
$this->shouldValidateSchema = $shouldValidateSchema;

parent::__construct($registry);
}
Expand Down Expand Up @@ -59,25 +63,28 @@ public function collect(Request $request, Response $response, Throwable $excepti

/** @var EntityManager $em */
foreach ($this->registry->getManagers() as $name => $em) {
$entities[$name] = [];
/** @var ClassMetadataFactory $factory */
$factory = $em->getMetadataFactory();
$validator = new SchemaValidator($em);

/** @var ClassMetadataInfo $class */
foreach ($factory->getLoadedMetadata() as $class) {
if (isset($entities[$name][$class->getName()])) {
continue;
}
if ($this->shouldValidateSchema) {
$entities[$name] = [];

$classErrors = $validator->validateClass($class);
$entities[$name][$class->getName()] = $class->getName();
/** @var ClassMetadataFactory $factory */
$factory = $em->getMetadataFactory();
$validator = new SchemaValidator($em);

if (empty($classErrors)) {
continue;
}
/** @var ClassMetadataInfo $class */
foreach ($factory->getLoadedMetadata() as $class) {
if (isset($entities[$name][$class->getName()])) {
continue;
}

$errors[$name][$class->getName()] = $classErrors;
$classErrors = $validator->validateClass($class);
$entities[$name][$class->getName()] = $class->getName();

if (empty($classErrors)) {
continue;
}

$errors[$name][$class->getName()] = $classErrors;
}
}

/** @var Configuration $emConfig */
Expand Down
4 changes: 4 additions & 0 deletions DependencyInjection/Configuration.php
View file
Open in desktop
Expand Up @@ -136,6 +136,10 @@ private function getDbalConnectionsNode()
->defaultValue(false)
->info('Enables collecting backtraces when profiling is enabled')
->end()
->booleanNode('profiling_collect_schema_errors')
ostrolucky marked this conversation as resolved.
Show resolved Hide resolved
->defaultValue(true)
->info('Enables collecting schema errors when profiling is enabled')
->end()
->scalarNode('server_version')->end()
->scalarNode('driver_class')->end()
->scalarNode('wrapper_class')->end()
Expand Down
10 changes: 8 additions & 2 deletions DependencyInjection/DoctrineExtension.php
View file
Open in desktop
Expand Up @@ -128,7 +128,9 @@ protected function loadDbalConnection($name, array $connection, ContainerBuilder
$profilingLoggerId = $profilingAbstractId . '.' . $name;
$container->setDefinition($profilingLoggerId, new ChildDefinition($profilingAbstractId));
$profilingLogger = new Reference($profilingLoggerId);
$container->getDefinition('data_collector.doctrine')->addMethodCall('addLogger', [$name, $profilingLogger]);
$container->getDefinition('data_collector.doctrine')
->addMethodCall('addLogger', [$name, $profilingLogger])
->replaceArgument(1, $connection['profiling_collect_schema_errors']);

if ($logger !== null) {
$chainLogger = new ChildDefinition('doctrine.dbal.logger.chain');
Expand All @@ -141,7 +143,11 @@ protected function loadDbalConnection($name, array $connection, ContainerBuilder
$logger = $profilingLogger;
}
}
unset($connection['profiling'], $connection['profiling_collect_backtrace']);
unset(
$connection['profiling'],
$connection['profiling_collect_backtrace'],
$connection['profiling_collect_schema_errors']
);

if (isset($connection['auto_commit'])) {
$configuration->addMethodCall('setAutoCommit', [$connection['auto_commit']]);
Expand Down
1 change: 1 addition & 0 deletions Resources/config/dbal.xml
View file
Open in desktop
Expand Up @@ -41,6 +41,7 @@
<service id="data_collector.doctrine" class="%doctrine.data_collector.class%" public="false">
<tag name="data_collector" template="@Doctrine/Collector/db.html.twig" id="db" priority="250" />
<argument type="service" id="doctrine" />
<argument>true</argument>
</service>

<service id="doctrine.dbal.connection_factory" class="%doctrine.dbal.connection_factory.class%">
Expand Down
1 change: 1 addition & 0 deletions Resources/config/schema/doctrine-1.0.xsd
View file
Open in desktop
Expand Up @@ -37,6 +37,7 @@
<xsd:attribute name="logging" type="xsd:string" default="false" />
<xsd:attribute name="profiling" type="xsd:string" default="false" />
<xsd:attribute name="profiling-collect-backtrace" type="xsd:string" default="false" />
<xsd:attribute name="profiling-collect-schema-errors" type="xsd:string" default="true" />
<xsd:attribute name="server-version" type="xsd:string" />
<xsd:attribute name="use-savepoints" type="xsd:boolean" />
<xsd:attributeGroup ref="driver-config" />
Expand Down
3 changes: 3 additions & 0 deletions Resources/doc/configuration.rst
View file
Open in desktop
Expand Up @@ -105,6 +105,8 @@ Configuration Reference
profiling: "%kernel.debug%"
# When true, profiling also collects a backtrace for each query
profiling_collect_backtrace: false
# When true, profiling also collects schema errors for each query
profiling_collect_schema_errors: true

server_version: ~
driver_class: ~
Expand Down Expand Up @@ -465,6 +467,7 @@ Configuration Reference
logging="%kernel.debug%"
profiling="%kernel.debug%"
profiling-collect-backtrace="false"
profiling-collect-schema-errors="true"
server-version=""
driver-class=""
wrapper-class=""
Expand Down
78 changes: 40 additions & 38 deletions Resources/views/Collector/db.html.twig
View file
Open in desktop
Expand Up @@ -387,47 +387,49 @@
{% endif %}
{% endif %}

<h2>Entities Mapping</h2>
{% if collector.entities|length > 0 %}
<h2>Entities Mapping</h2>

{% for manager, classes in collector.entities %}
{% if collector.managers|length > 1 %}
<h3>{{ manager }} <small>entity manager</small></h3>
{% endif %}
{% for manager, classes in collector.entities %}
{% if collector.managers|length > 1 %}
<h3>{{ manager }} <small>entity manager</small></h3>
{% endif %}

{% if classes is empty %}
<div class="empty">
<p>No loaded entities.</p>
</div>
{% else %}
<table>
<thead>
<tr>
<th scope="col">Class</th>
<th scope="col">Mapping errors</th>
</tr>
</thead>
<tbody>
{% for class in classes %}
{% set contains_errors = collector.mappingErrors[manager] is defined and collector.mappingErrors[manager][class] is defined %}
<tr class="{{ contains_errors ? 'status-error' }}">
<td>{{ class }}</td>
<td class="font-normal">
{% if contains_errors %}
<ul>
{% for error in collector.mappingErrors[manager][class] %}
<li>{{ error }}</li>
{% endfor %}
</ul>
{% else %}
No errors.
{% endif %}
</td>
{% if classes is empty %}
<div class="empty">
<p>No loaded entities.</p>
</div>
{% else %}
<table>
<thead>
<tr>
<th scope="col">Class</th>
<th scope="col">Mapping errors</th>
</tr>
{% endfor %}
</tbody>
</table>
{% endif %}
{% endfor %}
</thead>
<tbody>
{% for class in classes %}
{% set contains_errors = collector.mappingErrors[manager] is defined and collector.mappingErrors[manager][class] is defined %}
<tr class="{{ contains_errors ? 'status-error' }}">
<td>{{ class }}</td>
<td class="font-normal">
{% if contains_errors %}
<ul>
{% for error in collector.mappingErrors[manager][class] %}
<li>{{ error }}</li>
{% endfor %}
</ul>
{% else %}
No errors.
{% endif %}
</td>
</tr>
{% endfor %}
</tbody>
</table>
{% endif %}
{% endfor %}
{% endif %}

<script type="text/javascript">//<![CDATA[
function explain(link) {
Expand Down
21 changes: 19 additions & 2 deletions Tests/DataCollector/DoctrineDataCollectorTest.php
View file
Open in desktop
Expand Up @@ -49,6 +49,23 @@ public function testCollectEntities()
$this->assertCount(2, $entities['default']);
}

public function testDoesNotCollectEntities() : void
{
$manager = $this->createMock('Doctrine\ORM\EntityManager');
$config = $this->createMock('Doctrine\ORM\Configuration');
$collector = $this->createCollector(['default' => $manager], false);

$manager->expects($this->never())
->method('getMetadataFactory');
$manager->method('getConfiguration')
->will($this->returnValue($config));

$collector->collect(new Request(), new Response());

$this->assertEmpty($collector->getMappingErrors());
$this->assertEmpty($collector->getEntities());
Copy link
Member

Choose a reason for hiding this comment

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

Asserting this doesn't make sense to me, any particular reason to do it? Using getMappingErrors would at least make some sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

public function testGetGroupedQueries()
{
$logger = $this->getMockBuilder('Doctrine\DBAL\Logging\DebugStack')->getMock();
Expand Down Expand Up @@ -105,7 +122,7 @@ private function createEntityMetadata($entityFQCN)
*
* @return DoctrineDataCollector
*/
private function createCollector(array $managers)
private function createCollector(array $managers, bool $shouldValidateSchema = true)
{
$registry = $this->getMockBuilder('Doctrine\Persistence\ManagerRegistry')->getMock();
$registry
Expand All @@ -121,6 +138,6 @@ private function createCollector(array $managers)
->method('getManagers')
->will($this->returnValue($managers));

return new DoctrineDataCollector($registry);
return new DoctrineDataCollector($registry, $shouldValidateSchema);
}
}
5 changes: 5 additions & 0 deletions Tests/DependencyInjection/Fixtures/config/xml/dbal_logging.xml
View file
Open in desktop
Expand Up @@ -34,6 +34,11 @@
name="none"
logging="false"
profiling="false" />
<connection
name="profile_without_schema_validator"
logging="false"
profiling="true"
profiling-collect-schema-errors="true" />
</dbal>
</config>
</srv:container>
4 changes: 4 additions & 0 deletions Tests/DependencyInjection/Fixtures/config/yml/dbal_logging.yml
View file
Open in desktop
Expand Up @@ -19,3 +19,7 @@ doctrine:
both:
logging: true
profiling: true
profile_without_schema_validator:
logging: false
profiling: true
profiling_collect_schema_errors: true