Conversation
category doctrine schema
zhibek
left a comment
zhibek
left a comment
There was a problem hiding this comment.
This is looking good overall!
I've listed some review points - I want you to check these, but mostly you're doing things the right way.
| 'orm_default' => [ | ||
| // directory where proxies will be stored. By default, this is in | ||
| // the `data` directory of your application | ||
| 'proxy_dir' => 'data/DoctrineORMModule/Proxy', |
There was a problem hiding this comment.
It's not mandatory to set this (system temporary directories are used by default, which have the correct permissions) and in our Docker environment we want to keep things simple.
When we do set paths in our config, we should use realpath(__DIR__ . ...) to make sure the path to the directory is correct regardless of how the application is run.
| @@ -0,0 +1,269 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Usually we'd .gitignore auto-generated files like this.
| 'exception_template' => 'error/index', | ||
| 'template_map' => [ | ||
| 'layout/layout' => __DIR__ . '/../view/layout/layout.phtml', | ||
| 'layout/layout' => __DIR__ . '/../../../view/layout/layout.phtml', |
There was a problem hiding this comment.
Generally we'll use the Application module to hold site-wide files (including the layout template, error templates, etc). There's no need to move the layout file out of the Application module.
| ], | ||
| ], | ||
| 'view_manager' => [ | ||
| 'display_not_found_reason' => true, |
There was a problem hiding this comment.
There is no need to duplicate keys here that we already have declared in the Application module. The keys from config of each module get merged together (they're not kept separately for each module). This applies to most of the keys in this view_manager config - you should remove them all, apart from the ones which are specific to the Category module, such as the 3 template_map files and the template_path_stack (which is specific to the Category module as it uses __DIR__ to refer to the Category module directory).
| { | ||
| private $entityManager; | ||
|
|
||
| public function __construct($serviceManager) |
There was a problem hiding this comment.
Rather than injecting the $serviceManager, you should inject directly the services you need - in this case $entityManager, although really you should create your own service in this module and inject that (with $entityManager injected into that service, instead of into this controller).
Injecting the $serviceManager is considered to be an anti-pattern.
| Controller\ProductController::class => Controller\ProductControllerFactory::class, | ||
| ], | ||
| ], | ||
| 'view_manager' => [ |
There was a problem hiding this comment.
See review for module.config.php for Category module. We don't need to repeat keys that exist in the Application module as all modules configs are merged together.
| @@ -0,0 +1,85 @@ | |||
| <?php | |||
There was a problem hiding this comment.
See reviews for controllers in Category module.
| @@ -0,0 +1,40 @@ | |||
| <?php | |||
There was a problem hiding this comment.
See reviews in Category module - same issue applies here.
| * @ORM\Column(type="integer") | ||
| * @var integer | ||
| */ | ||
| public $category_id; |
There was a problem hiding this comment.
Use camelCase ($categoryId) for Doctrine entity properties.
| return $this->id; | ||
| } | ||
|
|
||
| public function setCategoryId($category_id) |
There was a problem hiding this comment.
Use camelcase ($categoryId) for arguments.
zhibek
left a comment
zhibek
left a comment
There was a problem hiding this comment.
Some more review points based on functional tests. Adding categories is working fine, but adding products is hitting an error.
| // Fill in the form with POST data | ||
| $data = $this->params()->fromPost(); | ||
| //var_dump($data); die; | ||
| $product->create($data); |
There was a problem hiding this comment.
I'm seeing a 500 error here when I run this. It appears the category isn't being set correctly.
| 'product-add-ajax' => [ | ||
| 'type' => Segment::class, | ||
| 'options' => [ | ||
| 'route' => '/product/post', |
There was a problem hiding this comment.
To keep things simple/obvious, it's usually best to name routes and actions the same way (i.e. either use addAjax for both the route name and the action -or- us post for both the route name and the action, not a mix of both).
Sometimes we'll need to break this rule for SEO or other reasons, but this isn't one of those times.
| @@ -0,0 +1 @@ | |||
| chmod -Rf 777 data/DoctrineORMModule/Proxy No newline at end of file | |||
There was a problem hiding this comment.
Having a deploy.sh (or doctrine-init.sh) file to automate setup/deployment is good. But it makes sense to include everything that's needed to setup the system, so adding the following line would be good too:
vendor/bin/doctrine-module orm:schema-tool:update --force
2 participants
No description provided.