Domain structure content blocks #2096

jeroendesloovere · 2017-06-14T08:14:00Z

Type

Enhancement

Resolves the following issues

New PR for #2091

Pull request description

The current ContentBlocks module in Fork CMS uses Doctrine and Symfony forms. That's amazing, but we are going a step further by dividing the logic in a folder called Domain. This is the way we are going to build all modules in Fork CMS. If you want to see some examples with the Domain folder, you can check out the MediaGalleries and MediaLibrary modules.

…ption

jonasdekeukelaere · 2017-06-14T12:04:38Z

Isn't moving everything into a Domain folder (with only Command, Event and Exception in it's own folder) making it more complicated to see what is what? Instead of clear folder names Command, DBALType, Entity, Form, Repository?
And why the extra ContentBlocks folder in the Domain folder?
Also why make the action class name unnecessary longer?
Those changes seem a bit unclear to me really.

carakas · 2017-06-14T12:22:01Z

@jeroendesloovere I think in this case I would keep the action classes the same as they are in fork 4
With block you have post and category, it makes sense there, but here it just confuses. Also I'm not convinced by the examples you provide, there is no confusion possible here like it is with the blog

apart from that I agree with your points.

carakas · 2017-06-14T12:24:52Z

@jonasdekeukelaere It makes more sense to group all things related to the entity in one parent namespace and separate that from the structure of the cms

If you have a category and a post for instance just adding everything in repository is the wrong approach imho (and also others) since you are grouping it wrong, The repository of the category has nothing to do with the repository of the post.

The reason I am for this is because this is a way simpler module so it is easier to point to then a more complex module like the media library.

The structure should be consistent

jonasdekeukelaere · 2017-06-14T12:27:14Z

For me it's more clearer to have (for example) all repositories, (or all Form types) in a folder together than have all those classes in 1 folder under Domain. Then you know directly what each class is suppose to do. But that sounds too easy maybe.

carakas · 2017-06-14T12:27:25Z

@jeroendesloovere I usually point to https://github.com/sumocoders/fork-partners/tree/master/src/Backend/Modules/Partners since it is a rather simple module. I also just used add edit delete there because it made sense

carakas · 2017-06-14T12:28:44Z

@jonasdekeukelaere no need to get passive aggressive

jonasdekeukelaere · 2017-06-14T12:29:47Z

For example the partners module, you have everything in this 1 folder https://github.com/sumocoders/fork-partners/tree/master/src/Backend/Modules/Partners/Domain/Partner - doesn't look right, really.

To be honest, this change feels like a step backwards somehow.

carakas · 2017-06-14T12:42:27Z

the problem with your approach @jonasdekeukelaere is that the main goal of namespaces is to group and avoid naming collisions.

If you have a post and a category, and both of them have a status value object but it is not the same value object
Then in your case you would need to change the proper name of the class to keep your file stucture while you should do it with namespaces.

When there are a lot of things of the same type withing a domain namespace I always add an extra sublevel, for instance with commands

jonasdekeukelaere · 2017-06-14T12:47:12Z

Then you can start making subfolders in your (for example) Repository or ValueObject folder.

carakas · 2017-06-14T12:51:32Z

removed since jeroens explanation was better

jeroendesloovere · 2017-06-14T12:51:49Z

@jonasdekeukelaere
When I first started using doctrine and symfony, Entity/Type/ValueObject/DBALTYpe folders were the best things I've ever heard of...
Then @WouterSioen and @carakas told me about a Domain structure.
At first, I was against it, just like you.
But then I decided to try it once and damn now I've already created a shitload of new modules/apps using the Domain structure. It's so much more logical.

Take a look at this "Realizations module" example:
I swear, "I'm never going back to Entity/Form/..." structure.

jonasdekeukelaere · 2017-06-14T12:55:52Z

Ok agree about that the entity is the common thing. But then I would make subfolders in the Domain/ContentBlock folder. Because now in @jeroendesloovere his example you just have a bunch of files that not really belong together. Some are Entity, some are DTO, some Form type, some Repository.

carakas

@jonasdekeukelaere that is because you can't see beyond what they are to you, a form, a DTO etc but they belong together, just like 2 wheels, a frame, a steer and some other things form a bike and are useless apart from each other (to a certain level)

carakas · 2017-06-14T12:56:12Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockAdd.php


 /**
 * This is the add-action, it will display a form to create a new item
 */
-class Add extends BackendBaseActionAdd
+class ContentBlockAdd extends BackendBaseActionAdd


I would keep this as Add if for some unforeseeable reason this would once in the future make the naming incorrect we can tackle it then, for now I would keep it at Add, Edit, Delete, and Index

carakas · 2017-06-14T12:57:09Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockAdd.php

@@ -43,27 +38,55 @@ public function execute(): void
        }

        /** @var CreateContentBlock $createContentBlock */


I don't think we need this anymore since the method has a return type and that gives us the autocompletion that this provided in the past

carakas · 2017-06-14T12:57:36Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockAdd.php

                [
-                    'report' => 'added',
+                    'report' => 'content-block-added',


why did you change this?

carakas · 2017-06-14T12:59:40Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockAdd.php

+        $form = $this->createForm(
+            ContentBlockType::class,
+            new CreateContentBlock(),
+            ['theme' => $this->get('fork.settings')->get('Core', 'theme', 'Core')]


see previous feedback, can you rebase on 5.0.0-dev because you are missing changes or fix this manually and check that you didn't occidentally remove something, I would personally prefer the rebase

carakas · 2017-06-14T13:00:11Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockDelete.php

+/**
+ * This is the delete-action, it will delete an item.
+ */
+class ContentBlockDelete extends BackendBaseActionDelete


same comment as above

jonasdekeukelaere · 2017-06-14T13:03:49Z

Sure they belong together, but for example all the code in 1 class belongs together as well, you will split that up into methods, right? For example the refactoring of all the installers. Why is that different with files and ok to just drop them al in 1 folder?

carakas · 2017-06-14T13:03:40Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockDelete.php

+        $this->redirect(
+            $this->getBackLink(
+                [
+                    'report' => 'content-block-deleted',


carakas · 2017-06-14T13:04:02Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockEdit.php

+    {
+        parent::execute();
+
+        /** @var ContentBlock $contentBlock */


carakas · 2017-06-14T13:04:05Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockEdit.php

+/**
+ * This is the edit-action, it will display a form to edit an existing item
+ */
+class ContentBlockEdit extends BackendBaseActionEdit


carakas · 2017-06-14T13:04:14Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockEdit.php

+            return;
+        }
+
+        /** @var UpdateContentBlock $updateContentBlock */


carakas · 2017-06-14T13:05:15Z

src/Backend/Modules/ContentBlocks/Actions/ContentBlockEdit.php

+            ContentBlockType::class,
+            new UpdateContentBlock($contentBlock),
+            [
+                'theme' => $this->get('fork.settings')->get('Core', 'theme', 'Core'),


carakas · 2017-06-14T13:23:20Z

src/Backend/Modules/ContentBlocks/Domain/ContentBlock/ContentBlock.php

 use Common\Locale;
 use DateTime;
 use Doctrine\ORM\Mapping as ORM;

 /**
 * @ORM\Table(name="content_blocks")
- * @ORM\Entity(repositoryClass="Backend\Modules\ContentBlocks\Repository\ContentBlockRepository")
+ * @ORM\Entity(repositoryClass="Backend\Modules\ContentBlocks\Domain\ContentBlock\ContentBlockRepository")


you should try it first, but I think because they are in the same namespace you can drop the FQCN and just use ContentBlockRepository

carakas · 2017-06-14T13:24:06Z

src/Backend/Modules/ContentBlocks/Domain/ContentBlock/ContentBlock.php

+        if ($dataTransferObject->hasExistingContentBlock()) {
+            $dataTransferObject->getContentBlockEntity()->status = Status::archived();
+
+            return self::create($dataTransferObject);


you repeat what is done below so you can just have the ifstatement and use one return here

carakas · 2017-06-14T13:31:25Z

src/Backend/Modules/ContentBlocks/Domain/ContentBlock/ContentBlockStatus.php

@@ -1,68 +0,0 @@
-<?php


can you rebase and actually remove these files in the same commit as you create the new class so that git sees it as a move?

@jeroendesloovere this shows as outdated but I need you to look at it :p

carakas · 2017-06-14T13:31:36Z

src/Backend/Modules/ContentBlocks/Domain/ContentBlock/ContentBlockStatusDBALType.php

-use Doctrine\DBAL\Types\Type;
-use Doctrine\DBAL\Platforms\AbstractPlatform;
-
-class ContentBlockStatusDBALType extends Type


carakas · 2017-06-14T13:32:01Z

src/Backend/Modules/ContentBlocks/Installer/Installer.php

@@ -35,8 +35,8 @@ private function configureBackendNavigation(): void
        $this->setNavigation(
            $navigationModulesId,
            $this->getModule(),
-            'content_blocks/index',
-            ['content_blocks/add', 'content_blocks/edit']
+            'content_blocks/content_block_index',


I would revert this

carakas

See comments, something went wrong and the comments got posted as normal comments

jeroendesloovere · 2017-06-14T14:18:49Z

@carakas thanks for your feedback. I've fixed it.

jeroendesloovere · 2017-06-14T14:20:17Z

Except merging forkcms 5.0.0 dev

jeroendesloovere · 2017-06-14T14:24:37Z

@carakas, can I rename the frontend action "Detail" to "ContentBlock"?

carakas · 2017-06-14T14:31:59Z

you can add a rename for the widget if you also provide the update query and info in the upgrade guide

jeroendesloovere added 28 commits June 14, 2017 09:22

ContentBlocksExtension: load('services.yml')

8f9ec14

Renamed Actions to "ContentBlockX" + updated locale for this

093f7ad

Config updated for ContentBlockIndex

75aaccc

Updated Layout file names for "ContentBlockX"

f452e17

info.xml updated with new version number and author

3e02dde

Moved DataGrid to /Domain/ContentBlock/

4de2ca8

Moved ContentBlockStatusDBALType to /Domain/ContentBlock

b23f96e

Renamed ContentBlockStatusDBALType to StatusDBALType

e945d63

Moved ValueObject/ContentBlockStatus to /Domain/ContentBlock

b268808

Renamed ContentBlockStatus to Status

4e4b3a7

Fixed doctrine.yml for ContentBlockStatus

733aa24

Moved commands to /Domain/ContentBlock

44929fe

Refactoring command and created ContentBlockDataTransferObject

b07865f

Moved events to /Domain/ContentBlock/Event

693f520

Event namespaces updated

3055c3e

Moved /Entity/ContentBlock to /Domain/ContentBlock

d79d13a

Refactored a little in ContentBlock

72afdb9

doctrine.yml updated for entity

d5c17d8

Moved /Repository/ContentBlockRepository.php to /Domain/ContentBlock

8a4197a

Refactored ContentBlockRepository and added ContentBlockNotFound Exce…

ee0829c

…ption

Moved /Form/ContentBlockType to /Domain/ContentBlock

7d2fc71

ContentBlockType: namespace updated

5248543

ContentBlockType: removed obsolete import

a9d8963

ContentBlockType: using ContentBlockDataTransferObject class

3ff62ba

Namespaces changed for DataGrids

026e325

Refactored datagrids (all datagrid logic is moved in the class)

2401c3c

Status has other class name

0758cd7

Installer updated

178e677

jeroendesloovere requested a review from carakas June 14, 2017 08:14

carakas reviewed Jun 14, 2017

View reviewed changes

carakas requested changes Jun 14, 2017

View reviewed changes

jeroendesloovere added 5 commits June 14, 2017 16:16

Reverting Action class names

6ccb89f

Moving setContentBlockEntity to the command and set the entity protected

b11eccf

Using @Orm\Entity(repositoryClass="ContentBlockRepository")

6892949

Removed obsolete self::create in ContentBlock

2c4de5f

PHPDoc updated

d291755

Merge branch 'forkcms/5.0.0-dev' into domain-structure-content-blocks

c35e1e0

"Fork" as default theme instead of "Core"

55379fa

carakas added this to the 5.0.0 milestone Jun 15, 2017

carakas approved these changes Jun 15, 2017

View reviewed changes

carakas merged commit 72ff9d9 into forkcms:5.0.0-dev Jun 15, 2017

jonasdekeukelaere mentioned this pull request Jun 16, 2017

Use POST request for deleting items #2090

Merged

		@@ -43,27 +38,55 @@ public function execute(): void
		}

		/** @var CreateContentBlock $createContentBlock */

Domain structure content blocks #2096

Domain structure content blocks #2096

Conversation

jeroendesloovere commented Jun 14, 2017

Type

Resolves the following issues

Pull request description

jonasdekeukelaere commented Jun 14, 2017

carakas commented Jun 14, 2017

carakas commented Jun 14, 2017

jonasdekeukelaere commented Jun 14, 2017

carakas commented Jun 14, 2017

carakas commented Jun 14, 2017

jonasdekeukelaere commented Jun 14, 2017 • edited Loading

carakas commented Jun 14, 2017

jonasdekeukelaere commented Jun 14, 2017

carakas commented Jun 14, 2017 • edited Loading

jeroendesloovere commented Jun 14, 2017 • edited Loading

jonasdekeukelaere commented Jun 14, 2017

carakas 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

Choose a reason for hiding this comment

jonasdekeukelaere commented Jun 14, 2017

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

carakas left a comment

Choose a reason for hiding this comment

jeroendesloovere commented Jun 14, 2017

jeroendesloovere commented Jun 14, 2017

jeroendesloovere commented Jun 14, 2017

carakas commented Jun 14, 2017

jonasdekeukelaere commented Jun 14, 2017 •

edited

Loading

carakas commented Jun 14, 2017 •

edited

Loading

jeroendesloovere commented Jun 14, 2017 •

edited

Loading