-
Notifications
You must be signed in to change notification settings - Fork 45
Import AcmeDemoBundle for dev-environment only #10
Conversation
@wouterj why do we even include this file like this? Should it not be the bundle that loads its services? |
@dantleech: there is a problem here. if we need to include this file at all, it should be in config_dev.yml. but i think we should not need to include the service file from a bundle. |
@dbu I wasn't aware that I closed this issue, must have done it accidentally .. sorry |
agreed with @dbu |
👍 for @dbu's suggestion too. In fact, we can just copy the Extension class from the core SE: https://github.com/sensiolabs/SensioDistributionBundle/blob/master/Resources/skeleton/acme-demo-bundle/Acme/DemoBundle/DependencyInjection/AcmeDemoExtension.php I'm not sure why I didn't do that in the first place. @phramz can you make this change? |
jep, that sounds like a good solution. +1 |
thanks .. now the last question is if services.yml should be switched to xml? |
No, the bundle is created for beginners only. We should use easy to understand code. Besides that, yaml is most known under beginners. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is not needed, let's remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ok .. i guess the bundle is also not intended for reuse |
app/config/config.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why you did this, but I'm not sure if it's good to do it this way. Configuring the template is part of the essential config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can just as well leave this here. maybe add a comment that the template is from the demo bundle and not available in production.
without this, the site will still not work in production mode, as there will be no controller found for a simplecms page document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that leaving this here always results in a "template not found" exception, since the logical template name cannot be resolved as the demo bundle is only registered in the dev env.
But this leaves me to another question: shouldn't we revert this change and register the demo bundle for all env?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought we follow the example of the symfony standard edition, but just noticed that the demo bundle does not contain any configurator. as long as its no security risk to run the demo bundle on a server, i would say lets move it to the prod part (but keep the di extension class added here so that we load the config the standard way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, yes it's a tricky situation ... it feels a bit funny having configs in prod for a bundle that should not be available in production. as long as we do not have kind of a "it works"-page for production it is probably better to leave the config were it was and revert that change. but in the end it wont make any difference as you will still get a "template not found" in prod.
Import AcmeDemoBundle for dev-environment only
thanks for the fix. i merged and created #11 to further discuss if the demo bundle should be available in production too. |
thanks |
The
AcmeDemoBundle
is dev-only (seeAppKernel
) and therefore not available in prod