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

Using Domain structure for ContentBlocks module #2091

Closed
wants to merge 1 commit into from
Closed

Using Domain structure for ContentBlocks module #2091

wants to merge 1 commit into from

Conversation

jeroendesloovere
Copy link
Member

@jeroendesloovere jeroendesloovere commented Jun 13, 2017 •
edited
Loading

Type

  • Enhancement

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.

@jeroendesloovere jeroendesloovere added this to the 5.0.0 milestone Jun 13, 2017
@mention-bot
Copy link

@jeroendesloovere, thanks for your PR! By analyzing the history of the files in this pull request, we identified @carakas, @tijsverkoyen and @WouterSioen to be potential reviewers.

@jeroendesloovere
Copy link
Member Author

jeroendesloovere commented Jun 13, 2017 •
edited
Loading

Note to self: saving a review isn't working
Edited: fixed

carakas
carakas requested changes Jun 13, 2017
View reviewed changes
src/Backend/Modules/ContentBlocks/Actions/ContentBlockAdd.php
);

$form->handleRequest($this->get('request'));
/** @var Form $form */
Copy link
Member

Choose a reason for hiding this comment

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

you don't need this phpdoc since the return type is already the form

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
Copy link
Member

Choose a reason for hiding this comment

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

I think in this case renaming this might be a bit overkill, but if you do it you need to make sure that all the changes needed to the database are added to the upgrade guide

Goes for all the other actions in this case as well.

src/Backend/Modules/ContentBlocks/Actions/ContentBlockAdd.php
$form = $this->createForm(
ContentBlockType::class,
new CreateContentBlock(),
['theme' => $this->get('fork.settings')->get('Core', 'theme', 'Core')]
Copy link
Member

Choose a reason for hiding this comment

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

This must be $this->get('fork.settings')->get('Core', 'theme', 'Fork')

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this has/will be fixed in another PR, so I won't change this. OK?

Copy link
Member

Choose a reason for hiding this comment

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

That PR has already been made and has been merged, so you should make sure you have the latests version of 5.0.0-dev, since git saw your changes as delete and add instead of move that change would've been lost

src/Backend/Modules/ContentBlocks/Actions/ContentBlockDelete.php
*/
class ContentBlockDelete extends BackendBaseActionDelete
{
public function execute(): void
Copy link
Member

Choose a reason for hiding this comment

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

if #2090 gets merged before this one the logic here needs to be updated since it won't get merged because you changed everything in one big commit and thereby are breaking the git history

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

$form->handleRequest($this->get('request'));
/** @var Form $form */
$form = $this->getForm();

if (!$form->isValid()) {
Copy link
Member

Choose a reason for hiding this comment

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

checking if a form is valid before checking if a form is submitted is deprecated in symfony 3.3 and removed in symfony 4, since we will need to upgrade for fork 5 this will need to be changed here as well

src/Backend/Modules/ContentBlocks/Domain/ContentBlock/ContentBlockRepository.php
@@ -38,20 +38,43 @@ public function getNextIdForLanguage(Locale $locale): int
->getSingleScalarResult() + 1;
}

public function findOneByIdAndLocale($id, Locale $locale): ?ContentBlock
public function findOneByIdAndLocale(int $id = null, Locale $locale): ?ContentBlock
Copy link
Member

Choose a reason for hiding this comment

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

wrong method signature, if the int needs to be able to receive a null (maybe check the implementation) you need to use ?int and not = null because now you have an optional parameter before a required one

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

public function findOneByRevisionIdAndLocale($revisionId, Locale $locale): ?ContentBlock
public function findOneByRevisionIdAndLocale(int $revisionId = null, Locale $locale): ContentBlock
Copy link
Member

Choose a reason for hiding this comment

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

likewise

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

public static function getHtml(ContentBlock $contentBlock, Locale $locale): string
{
return (string) (new self($contentBlock, $locale))->getContent();
Copy link
Member

Choose a reason for hiding this comment

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

we no longer need to cast to string since ->getContent() now always will return a string and no longer null but '' when there are no results

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

use Exception;

class ContentBlockNotFound extends Exception
Copy link
Member

Choose a reason for hiding this comment

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

👍

src/Backend/Modules/ContentBlocks/Layout/Templates/ContentBlockEdit.html.twig
@@ -51,7 +51,7 @@
<div class="col-md-12">
<div class="btn-toolbar">
<div class="btn-group pull-left" role="group">
{% if showContentBlocksDelete %}
{% if showContentBlocksContentBlockDelete %}
Copy link
Member

Choose a reason for hiding this comment

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

see with the renaming of the actions in this case it only complicates reading things like this

@carakas
Copy link
Member

carakas commented Jun 13, 2017

I'm going to close this one like we discussed since too much of the git history is broken.
Please look at the feedback and create a new PR with nice descriptive commits and doing one step at a time

@carakas carakas closed this Jun 13, 2017
@carakas carakas removed this from the 5.0.0 milestone Jun 13, 2017
@jeroendesloovere jeroendesloovere mentioned this pull request Jun 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carakas carakas carakas requested changes

Assignees
No one assigned
Labels
ContentBlocks Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jeroendesloovere @mention-bot @carakas