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.

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

[shopsys] entities: removal of dependencies on services #1123

Merged
merged 25 commits into from Jun 26, 2019

Conversation

@vitek-rostislav
Copy link
Contributor

vitek-rostislav commented Jun 10, 2019

Q A
Description, reason for the PR There is a new rule in Shopsys Framework - the logic belongs to entities only in such a case when there is not needed any external dependency (ie. entity has all the relevant data available), otherwise, the logic is in facades (resp. the facades are used as delegates to other services). This is based on the feedback from implementation teams - in the previous state, it was difficult to work with entities when there were dependencies on DI services (the services are required as arguments in some methods) or to extend such services. Moreover, it was unclear where the logic belongs.
New feature No
BC breaks Yes - many methods have changed signature, or were (re)moved - there will be a special doc with upgrading instructions
Fixes issues ...
Have you read and signed our License Agreement for contributions? Yes
@vitek-rostislav vitek-rostislav force-pushed the rv-entities-without-services branch 2 times, most recently from c662e65 to 0f75f84 Jun 13, 2019
@TomasLudvik TomasLudvik force-pushed the rv-entities-without-services branch from 8b29b95 to 4ffcf68 Jun 17, 2019
@vitek-rostislav vitek-rostislav force-pushed the rv-entities-without-services branch 5 times, most recently from 79b423d to 2c60ed9 Jun 17, 2019
@vitek-rostislav vitek-rostislav marked this pull request as ready for review Jun 18, 2019
Copy link
Contributor

boris-brtan left a comment

THX for the PR,
can you please also check the commit messages?
in my git client it looks like the fist row is sometimes broken into 2 lines.

@simara-svatopluk

This comment has been minimized.

Copy link
Contributor

simara-svatopluk commented Jun 18, 2019

Hi, I promised I won't give any points against this decision, but model&modeling is my passion so I'm writing again.

I attended PHPce Doctrine+DDD workshop last year and https://github.com/Ocramius recommended our current "passing dependencies to entities" approach. This PR is against the recommendation.
Just take a look https://github.com/ShittySoft/php-ce-2018-doctrine-tutorial/blob/feature/aggregate-with-domain-services/src/Authentication/Aggregate/User.php#L49

Please consider that even this programmer (Doctrine authority) recommends passing dependencies to methods of entities at least during CR and BV.

@vitek-rostislav

This comment has been minimized.

Copy link
Contributor Author

vitek-rostislav commented Jun 19, 2019

Hi @boris-brtan, thanks for the review! I answered and fixed all the notes with fixup commits, please check them out 🙂

You are right about the weird breaks in commit messages, I will fix it during rebase, thanks for noticing!

Hi @simara-svatopluk, thanks for your intervention, I believe you have a valid point, however, you know that the voices that required this change were very strong and I am just trying to respect the wish of our users 🙂

@pk16011990

This comment has been minimized.

Copy link
Member

pk16011990 commented Jun 19, 2019

@simara-svatopluk We disscussed about it...all present agreed, do you remember?
And extending of entities (basic philosophi of SSFW) are really hard without this change -> Imagine that I need to add new bussines logic for editing of entity, I can't add new mandatory parameter (new service) to edit method of entity.

Yes, It can be against the recommendation, but SSFW sometimes abuse inheritence and there is the place where we do it.

@vitek-rostislav vitek-rostislav force-pushed the rv-entities-without-services branch 4 times, most recently from 3efabf8 to 32bae90 Jun 20, 2019
Copy link
Contributor

boris-brtan left a comment

Hello,
THX for the rebase and fixes, i just have a few suggestions and questions about upgrade notes

…de instead of Order entity

- external services are needed so the logic must not be in the entity
…o OrderFacade

- entities must not use external dependencies in their logic
- entities must not use external dependencies in their logic
 - entities must not use external dependencies in their logic
- entities must not use external dependencies in their logic
- entities must not use external dependencies in their logic
- entities must not use external dependencies in their logic
- it is not possible to create QuantifiedProduct without Product instance
- if $product was null, then get_class($product) would return unexpected result
…entCartWithCart()

- entities must not use external dependencies in their logic
- entities must not use external dependencies in their logic
- entities must not use external dependencies in their logic
- the creation of ProductCategoryDomain entities is moved to ProductCategoryDomainFactory and used in ProductFacade
- Product::$productCategoryDomains now must be set from outside (public setter)
- when setting $productCategoryDomains array for main variant, the items are copied for all its variants (ProductCategoryDomain::$product must be changed accordingly using public setter)
- when adding a new variant, the $productCategoryDomains are copied from the main variant the same way as well
…eduler

- the recalculation scheduling is now called from ProductFacade
- entities must not use external dependencies in their logic
…d to new ProductInputPriceRecalculator

- entities must not use external dependencies in their logic
…soluteUrlByFriendlyUrl()

- entities must not use external dependencies in their logic
- FriendlyUrlTest removed for little added value
…tomerFacade::setEmail()

- checking for duplicate email is simplified a little bit
- entities must not use external dependencies in their logic
…assword()

- User::setNewPasssword() logic moved to CustomerPasswordFacade::setNewPassword()
- entities must not use external dependencies in their logic
…stead of instance of HashGenerator

- entities must not use external dependencies in their logic
…ess()

- entities must not use external dependencies in their logic
…rDelete()

- entities must not use external dependencies in their logic
…ic moved to AdministratorFacade::setPassword()

- entities must not use external dependencies in their logic
…istratorFacade::checkUsername()

- entities must not use external dependencies in their logic
- entities must not use external dependencies in their logic
@vitek-rostislav vitek-rostislav force-pushed the rv-entities-without-services branch from c908081 to c8b87bf Jun 26, 2019
@vitek-rostislav vitek-rostislav changed the title [framework] entities: removal of dependencies on services entities: removal of dependencies on services Jun 26, 2019
@vitek-rostislav vitek-rostislav merged commit ef87f89 into 8.0 Jun 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vitek-rostislav vitek-rostislav deleted the rv-entities-without-services branch Jun 26, 2019
@PetrHeinz PetrHeinz changed the title entities: removal of dependencies on services [shopsys] entities: removal of dependencies on services Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.