tdutrion
commented
5 years ago Thanks for this very first contribution!
Despite all my comments on a great deal of small "issues", I'd be happy to process and merge in a very near future and then continue with other contexts.
I hope my comments won't drive you off. There are two main points:
- consistency (are we looking the same as the rest of the code, except for the organisation change which is the purpose of the PR)
- maintenance (comments), which are only needed in PHP for throw and array type, since we have no generics nor throws keyword.
There is also a couple of changes that I believe do not belong to this PR.
Thanks again for the work you did so far!
kuraobi
PR information
Description
This is a proof of concept for a more modular code organization. I refactored code for Talks, with the different use cases being index (list), create, update, delete, show. This is a very simple example, but gives a good preview of the target code structure. I'll try to explain the main aspects.
Namespacing and naming
All the code related to Talks should be in the
App\TalkNamespace. Classes are then organized around each feature, i.e.Create,Delete,Index,UpdateandShow. Namespaces describing the type of objects inside (Controller, Entity, ValueObject...) are a very low form of cohesion and are highly discouraged, as they go against a functional structure. As a result, this information is better conveyed with suffixes:App\Controller\Talk\CreatebecomesApp\Talk\Create\CreateTalkActionI also used a more complete class name (
CreateTalkActionvsCreate) because it makes code easier to read, it prevents name collision and the necessity for using aliases, and it prevents refactoring errors, but this one is maybe more a personal choice.Configuration files (DI and routes)
Configuration files are also grouped by domain, which means each domain can have its own configuration files. This means a
services.yaml, adoctrine.yamland aroutes.yamlfile in most cases. I have located these files in aResourcesdirectory insidesrc/Talkbecause it's convenient and in line with how Symfony bundles are organized, which could be useful if our modules evolve into bundles later on. That being said, one could argue thatsrcshould be code only while all config should go toconfig, in which case it would also be feasible to replicate this structure inside theconfigdirectory.This requires a few lines in the
App\Kernelto allow detection of config and route files in the right directories.Dependency Injection
One drawback of a functional organization of code is that we basically can use the
resourceoption to declare and configure services in Symfony DI. So we need to explicitly list the classes that we want to set as services.Tagging of Actions
A new
App\Actionmarker interface has been created to allow easy selection of all actions in theservices.yamlfile, to add thecontroller.service_argumentstag required. Without the interface, we would have to add the tag on each one of the actionsDoctrine
Doctrine being an implementation, It is kept separated from the rest of the code. This includes the repositories implementations, the fixtures classes, and also the mappings, which are switched to XML for this purpose.
Misc
Having
createmethods into theTalkRepositoryreally seemed off to me, as I had a dependency between the repository and theRequestobject coming from the form. I chose to move them to a separateTalkFactory, which I added into theApp\Talk\Createnamespace.