Make a db_driver configuration parameter optional with no_driver default value #1420

covex-nn · 2018-04-20T07:28:40Z

I am targeting this branch, because this is BC.

Changelog

### Changed
- A db_driver configuration parameter is optional now with 'no_driver' default value

To do

Add tests

Subject

To create a recipe for Symfony Flex, it should be possible to create a default configuration, that allows to run cache:clear without errors (see sonata-project/dev-kit#409) A default value for db_driver configuration parameter will allow this.

If this PR will be accepted, a default configuration, i.e. a recipe for sonata-media/media-bundle could be like this (please fix me, it there something wrong here; may be providers or formats sections should be changed to more commonly used?):

sonata_media:
    default_context: default
    contexts:
        default:
            providers:
                - sonata.media.provider.dailymotion
                - sonata.media.provider.youtube
                - sonata.media.provider.image
                - sonata.media.provider.file
                - sonata.media.provider.vimeo

            formats:
                small: { width: 100 , quality: 70}
                big:   { width: 500 , quality: 70}

    cdn:
        server:
            path: /upload/media

    filesystem:
        local:
            directory: "%kernel.project_dir%/public/upload/media"
            create: false

Additional configuration, i.e. a recipe for a new package sonata-project/media-orm-pack could be like this (i created a gist with entity classes):

sonata_media:
    class:
        media: App\Entity\SonataMediaMedia
        gallery: App\Entity\SonataMediaGallery
        gallery_has_media: App\Entity\SonataMediaGalleryHasMedia
    db_driver: doctrine_orm

kunicmarko20 · 2018-04-20T07:33:38Z

src/Generator/NoDriverGenerator.php

+
+use Sonata\MediaBundle\Model\MediaInterface;
+use Sonata\MediaBundle\SonataMediaBundle;
+


@author is missing

kunicmarko20 · 2018-04-20T07:33:58Z

src/Model/NoDriverManager.php

+/**
+ * @internal
+ */
+class NoDriverManager implements ManagerInterface


kunicmarko20 · 2018-04-20T07:34:16Z

src/Model/NoDriverManager.php

+        SonataMediaBundle::noDriverRuntimeException();
+    }
+
+    public function findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)


please add comments about types of vars

kunicmarko20 · 2018-04-20T07:34:24Z

src/Model/NoDriverManager.php

+        SonataMediaBundle::noDriverRuntimeException();
+    }
+
+    public function find($id)


comment about type

kunicmarko20 · 2018-04-20T07:34:29Z

src/Model/NoDriverManager.php

+        SonataMediaBundle::noDriverRuntimeException();
+    }
+
+    public function save($entity, $andFlush = true)


comment about type

kunicmarko20 · 2018-04-20T07:34:32Z

src/Model/NoDriverManager.php

+        SonataMediaBundle::noDriverRuntimeException();
+    }
+
+    public function delete($entity, $andFlush = true)


comment about type

kunicmarko20 · 2018-04-20T07:35:34Z

src/DependencyInjection/SonataMediaExtension.php

@@ -99,7 +99,7 @@ public function load(array $configs, ContainerBuilder $container)
        $loader->load('serializer.xml');
        $loader->load('command.xml');

-        if (!in_array(strtolower($config['db_driver']), ['doctrine_orm', 'doctrine_mongodb', 'doctrine_phpcr'])) {
+        if (!in_array(strtolower($config['db_driver']), ['doctrine_orm', 'doctrine_mongodb', 'doctrine_phpcr', 'no_driver'])) {


can we extract this array into a const maybe, what do @sonata-project/contributors think?

Here is how it is done in FOSUserBundle, we can move this check to Sonata\NotificationBundle\DependencyInjection\Configuration and add constant there

Ye, I would like that more, let's see what others say.

Sounds better, if we don't produce any BC breaks I would prefer that way too

ah, forgot about this @covex-nn can you move this without doing a BC break?

Sure! i'll do it a little bit later (i must go right now)

Blocked merge, thank you for helping!

@kunicmarko20 all done, please review )

jordisala1991 · 2018-04-20T08:07:15Z

src/Model/NoDriverManager.php

+
+    public function findAll()
+    {
+        SonataMediaBundle::noDriverRuntimeException();


Why don't we generate a new Exception class for this?

Yes, sounds better.

This error will be shown to developer in a browser, if a pack with additional configuration was not installed. Here is a screenshot how it is decorated by symfony/debug in dev environment:

That message could be updated! We could add instructions how to install a pack =)
But, i agree, that method should be removed. Can i use Sonata\CoreBundle\Exception\InvalidParameterException?

I thought more about new exception class, NoDriverException?

Added new NoDriverException

kunicmarko20 · 2018-04-20T09:06:38Z

tests/Model/NoDriverManagerTest.php

+    {
+        $this->expectException('Sonata\MediaBundle\Exception\NoDriverException');
+
+        $manager = new NoDriverManager();


this var is not needed, can you just move new NoDriverManager() to method?

kunicmarko20 · 2018-04-20T09:06:49Z

tests/Model/NoDriverManagerTest.php

+     */
+    public function testException($method, array $arguments)
+    {
+        $this->expectException('Sonata\MediaBundle\Exception\NoDriverException');


Can you use NoDriverException::class

kunicmarko20 · 2018-04-20T09:07:09Z

tests/Generator/NoDriverGeneratorTest.php

+{
+    public function testException()
+    {
+        $this->expectException('Sonata\MediaBundle\Exception\NoDriverException');


same here, use ::class

kunicmarko20

blocked to avoid merge

…ult value

kunicmarko20 · 2018-04-20T13:42:52Z

src/DependencyInjection/Configuration.php

+                    ->defaultValue('no_driver')
+                    ->validate()
+                        ->ifNotInArray(self::DB_DRIVERS)
+                        ->thenInvalid('SonataMediaBundle - Invalid db driver %s.')


will it throw the same exception?

Before my changes there was \InvalidArgumentException, and now it will throw a same exception, see https://github.com/symfony/config/blob/master/Definition/Builder/ExprBuilder.php#L187, but before container compilation started

kunicmarko20

Great work!

core23 · 2018-04-20T13:55:09Z

src/Exception/NoDriverException.php

+/**
+ * @author Andrey F. Mindubaev <[email protected]>
+ */
+class NoDriverException extends \RuntimeException


core23 · 2018-04-20T13:55:22Z

src/Generator/NoDriverGenerator.php

+ *
+ * @author Andrey F. Mindubaev <[email protected]>
+ */
+class NoDriverGenerator implements GeneratorInterface


core23 · 2018-04-20T13:55:27Z

src/Model/NoDriverManager.php

+ *
+ * @author Andrey F. Mindubaev <[email protected]>
+ */
+class NoDriverManager implements ManagerInterface


covex-nn · 2018-04-24T15:45:14Z

I pushed a commit with recipe for MediaBundle; after new version will be released i will create a PR

See covex-nn/recipes-contrib@master...sonata-media
If there something i should add (like url to docs, or that tip about 'quality' that it should not be greater than 100 for png) - please tell me

kunicmarko20 · 2018-04-24T15:50:43Z

I think we need to mention that uploads folder should be writable

greg0ire · 2018-04-24T19:35:03Z

src/DependencyInjection/Configuration.php

@@ -32,7 +37,13 @@ public function getConfigTreeBuilder()

        $node
            ->children()
-                ->scalarNode('db_driver')->isRequired()->end()
+                ->scalarNode('db_driver')
+                    ->defaultValue('no_driver')


Please add a call to info()

Added ->info('Choose persistence mechanism driver from the following list: "doctrine_orm", "doctrine_mongodb", "doctrine_phpcr"')

greg0ire · 2018-04-24T19:36:21Z

src/Exception/NoDriverException.php

+{
+    const DEFAULT_MESSAGE = 'The child node "db_driver" at path "sonata_media" must be configured.';
+
+    public function __construct($message = self::DEFAULT_MESSAGE, $code = 0, \Throwable $previous = null)


\Throwable is not available with php 5

greg0ire · 2018-04-24T19:37:29Z

src/Exception/NoDriverException.php

+ */
+final class NoDriverException extends \RuntimeException
+{
+    const DEFAULT_MESSAGE = 'The child node "db_driver" at path "sonata_media" must be configured.';


Why put this in a constant? It's public now, and it's only ever used once.

This is a default value for message. I set it once and use it each time when i throw this exception just with new NoDriverException(); without setting first argument.

Yeah but why not do this: https://3v4l.org/WmvS8 or this: https://3v4l.org/ROnrq?

Just i did not think, that it is important. Next time i will try not to create new not necessary entities. Fixed

greg0ire · 2018-04-24T19:39:07Z

src/Resources/config/no_driver.xml

+    <services>
+        <service id="sonata.media.manager.media" class="Sonata\MediaBundle\Model\NoDriverManager"/>
+        <service id="sonata.media.manager.gallery" class="Sonata\MediaBundle\Model\NoDriverManager"/>
+        <service id="sonata.media.generator.default" class="Sonata\MediaBundle\Generator\NoDriverGenerator"/>


Please mark service than can be private as such

@greg0ire only sonata.media.generator.default can be private, two others must be public. I added public="true" there

greg0ire · 2018-04-24T19:39:24Z

src/Resources/config/no_driver_admin.xml

@@ -0,0 +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+</container>


This file is loaded from SonataMediaExtension.php when SonataAdminBundle is enabled. So, i created an empty file for that case. <container /> tag can be empty. See http://symfony.com/schema/dic/services/services-1.0.xsd

Please add an XML comment to explain this, hopefully xmllint will not remove it.

greg0ire · 2018-04-25T21:26:25Z

src/Resources/config/no_driver_admin.xml

@@ -1,3 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+    <!--
+     This file will be loaded when SonataAdminBundle is enabled and db_driver has it's default value, not corresponding to real persistence mechanism.


it's => its

linebreak that line 4

and we're good

greg0ire · 2018-04-26T06:55:55Z

Thanks @covex-nn !

covex-nn mentioned this pull request Apr 20, 2018

Recipes for Symfony Flex sonata-project/dev-kit#409

Closed

26 tasks

kunicmarko20 reviewed Apr 20, 2018

View reviewed changes

src/Model/NoDriverManager.php Outdated

/**

* @internal

*/

class NoDriverManager implements ManagerInterface

Copy link

Contributor

kunicmarko20 Apr 20, 2018

Choose a reason for hiding this comment

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

@author

kunicmarko20 reviewed Apr 20, 2018

View reviewed changes

OskarStark added the minor label Apr 20, 2018

jordisala1991 reviewed Apr 20, 2018

View reviewed changes

kunicmarko20 reviewed Apr 20, 2018

View reviewed changes

kunicmarko20 approved these changes Apr 20, 2018

View reviewed changes

kunicmarko20 suggested changes Apr 20, 2018

View reviewed changes

covex-nn added 2 commits April 20, 2018 16:08

Make a db_driver configuration parameter optional with no_driver defa…

8218af2

…ult value

Move db_config validation to DependencyInjection\Configuration

1a628f0

covex-nn force-pushed the db-driver branch from a0fba4e to 1a628f0 Compare April 20, 2018 13:09

kunicmarko20 reviewed Apr 20, 2018

View reviewed changes

kunicmarko20 previously approved these changes Apr 20, 2018

View reviewed changes

kunicmarko20 requested a review from a team April 20, 2018 13:49

core23 reviewed Apr 20, 2018

View reviewed changes

covex-nn dismissed kunicmarko20’s stale review via 5cccbab April 20, 2018 14:02

kunicmarko20 approved these changes Apr 20, 2018

View reviewed changes

kunicmarko20 previously approved these changes Apr 23, 2018

View reviewed changes

kunicmarko20 requested review from jordisala1991 and core23 and removed request for core23 April 23, 2018 11:27

kunicmarko20 requested a review from OskarStark April 23, 2018 11:27

jordisala1991 previously approved these changes Apr 23, 2018

View reviewed changes

kunicmarko20 requested a review from greg0ire April 24, 2018 06:37

greg0ire requested changes Apr 24, 2018

View reviewed changes

covex-nn dismissed stale reviews from jordisala1991 and kunicmarko20 via f602634 April 25, 2018 12:42

greg0ire reviewed Apr 25, 2018

View reviewed changes

greg0ire previously approved these changes Apr 25, 2018

View reviewed changes

covex-nn dismissed greg0ire’s stale review via 238994d April 25, 2018 21:35

Some fixes

4294761

covex-nn force-pushed the db-driver branch from 1f420ad to 4294761 Compare April 26, 2018 05:14

Fix ?? for php5.6

b2f479b

covex-nn force-pushed the db-driver branch from 73b55b4 to b2f479b Compare April 26, 2018 05:25

greg0ire approved these changes Apr 26, 2018

View reviewed changes

jordisala1991 approved these changes Apr 26, 2018

View reviewed changes

kunicmarko20 approved these changes Apr 26, 2018

View reviewed changes

greg0ire merged commit 4e9be09 into sonata-project:3.x Apr 26, 2018

covex-nn deleted the db-driver branch April 26, 2018 07:05

covex-nn mentioned this pull request Apr 26, 2018

[RFC] Structure of Flex-recipe for sonata-project bundles sonata-project/dev-kit#397

Closed

covex-nn mentioned this pull request May 28, 2018

Make db_driver parameter optional with "no_driver" default value FriendsOfSymfony/FOSUserBundle#2708

Open


		use Sonata\MediaBundle\Model\MediaInterface;
		use Sonata\MediaBundle\SonataMediaBundle;

Make a db_driver configuration parameter optional with no_driver default value #1420

Make a db_driver configuration parameter optional with no_driver default value #1420

Conversation

covex-nn commented Apr 20, 2018 • edited Loading

Changelog

To do

Subject

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunicmarko20 Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunicmarko20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunicmarko20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

covex-nn commented Apr 24, 2018

kunicmarko20 commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

covex-nn Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

covex-nn Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

covex-nn Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

covex-nn Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

greg0ire Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Apr 26, 2018

covex-nn commented Apr 20, 2018 •

edited

Loading

kunicmarko20 Apr 20, 2018 •

edited

Loading

covex-nn Apr 25, 2018 •

edited

Loading

covex-nn Apr 25, 2018 •

edited

Loading

covex-nn Apr 25, 2018 •

edited

Loading

covex-nn Apr 25, 2018 •

edited

Loading

greg0ire Apr 25, 2018 •

edited

Loading