Skip to content

Conversation

mcg-web
Copy link
Contributor

@mcg-web mcg-web commented Sep 21, 2017

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Documented? yes
Fixed tickets #189
License MIT

Copy link
Contributor

@renatomefi renatomefi left a comment

Choose a reason for hiding this comment

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

Forgive my ignorance on the bundle, but when is it necessary to use the this feature? Only when creating custom types or so?

if (null === $mapClassLoader) {
$mapClassLoader = new ClassLoader();
$mapClassLoader->setClassMapAuthoritative(true);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to enter this else statement? $mapClassLoader seems to be always null to me or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it is null on init because $mapClassLoader is declare as static so after it init it always has the same instance of ClassLoader.

env: SYMFONY_VERSION=3.2.*
- php: 7.1
env: SYMFONY_VERSION=3.3.* SCRUTINIZER=true
env: SYMFONY_VERSION=3.3.* TEST_COVERAGE=true
Copy link
Contributor

Choose a reason for hiding this comment

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

cool 👍

{
$config = $container->getParameter('overblog_graphql_types.config');
$generatedClasses = $container->get('overblog_graphql.cache_compiler')->compile($this->processConfig($config));
$generatedClasses = $container->get('overblog_graphql.cache_compiler')->compile(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea to extract $container->get('overblog_graphql.cache_compiler') and check its type just for safety and the IDE will also type hint it nicely

Copy link
Contributor

@barduck007 barduck007 left a comment

Choose a reason for hiding this comment

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

LGTM

@mcg-web mcg-web merged commit 64486c7 into overblog:master Sep 25, 2017
@mcg-web mcg-web deleted the optimizations branch September 25, 2017 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants