IBX-952: Added chosen Location to Translation Choice form #1885

barw4 · 2021-09-02T16:12:33Z

Question	Answer
Tickets	IBX-952
Bug fix?	yes
New feature?	no
BC breaks?	no
Tests pass?	yes
Doc needed?	no
License	GPL-2.0

We should consider the Location we are actually in to load the required languages as the user might not have access to main Location but have access to secondary one.

Related ezplatform-page-builder PR: https://github.com/ezsystems/ezplatform-page-builder/pull/826.

Checklist:

Coding standards ($ composer fix-cs)
Ready for Code Review

mikadamczyk

I would say that because the user might not have access to the main location we should not treat it like a secure fallback. If in any of those cases location will be null then an error will occur.

mikadamczyk · 2021-09-03T06:49:11Z

src/lib/Form/Type/ChoiceList/Loader/ContentEditTranslationChoiceLoader.php

@@ -85,7 +91,11 @@ function (Language $language) {
                $this->contentInfo,
                [
                    (new Target\Builder\VersionBuilder())->translateToAnyLanguageOf($languagesCodes)->build(),
-                    $this->locationService->loadLocation($this->contentInfo->mainLocationId),
+                    $this->locationService->loadLocation(
+                        $this->location !== null


If we will have a location there is a need to reload it?

I believe this ChoiceLoader is not a service, it's initialized with the form so it shouldn't really be a problem.

ContentEditTranslationChoiceLoader is a service. Of course, we could reload but what for? It looks not so good

If this is indeed a service then sure we shouldn't store it as a property 👍

@mikadamczyk It's not a service. It's built on demand, just like most choice loaders do.

I guess the issue could be that the current user does not have access to a Location in current context, hence the reload. I do think though that it should be skipped if it's passed directly - relying on implicit security check would be bad anyway.

As we agreed, it is incorrectly registered as a service

mikadamczyk · 2021-09-03T06:52:02Z

src/lib/Form/Type/ChoiceList/Loader/ContentEditTranslationChoiceLoader.php

@@ -50,14 +54,16 @@ public function __construct(
        ?ContentInfo $contentInfo,
        LookupLimitationsTransformer $lookupLimitationsTransformer,
        array $languageCodes,
-        LocationService $locationService
+        LocationService $locationService,
+        ?Location $location


Suggested change

?Location $location

?Location $location = null

Does it make sense to always require this parameter and drop the LocationService dependency as a result?

Aside from BC, I think this would be a good idea.

webhdx · 2021-09-03T07:05:36Z

@mikadamczyk You are right, this is not fixing the root issue, only decreases chances of happening. Anyway this is still a valid PR because it makes the form being closer to actual context and not just rely on possibly completely different Location.

Steveb-p

There are 2 questions posted in comments that I'd like to have answered before merging:

Do we reload the Location object internally, if it has been passed directly via constructor?
Do we expect to make $location argument mandatory in Choice Loader? If yes, then should we deprecate $locationService argument (as it's only purpose was to do the loading)?

Additionally, should we remove the ChoiceLoader service, as it should not (and cannot) be used as a service? If yes, then should it be a separate PR, or should we do so immediately?

barw4 · 2021-09-03T13:51:37Z

@Steveb-p
when it comes to question 1 I think we can skip reloading the Location, the same Location is still checked when accessing this content before.
2: Mandatory with null as a default or without null? Based on the parts in system where this service is called, we cannot guarantee we will have Location instead of null. If that's a null we won't really be able to load limitations for the given content, that's why we could help ourselves with mainLocationId which in fact should be loaded.

Steveb-p · 2021-09-03T14:21:03Z

@Steveb-p
when it comes to question 1 I think we can skip reloading the Location, the same Location is still checked when accessing this content before.
2: Mandatory with null as a default or without null? Based on the parts in system where this service is called, we cannot guarantee we will have Location instead of null. If that's a null we won't really be able to load limitations for the given content, that's why we could help ourselves with mainLocationId which in fact should be loaded.

Yeah, it's impossible to get to that location (see what I did there...?) without getting Location in current site access / user context. So it should be skipped.
null as default, because we can't make it mandatory without a BC break. In terms of future direction that we could go is making it mandatory to pass Location from calling code (i.e. when constructing the ChoiceLoader in question). This would then lead to removal of LocationService dependency in ChoiceLoader itself - instead developers would use LocationService in calling code as necessary.

🧙‍♂️ Summoning @alongosz for architecture design decision

alongosz

@Steveb-p

null as default, because we can't make it mandatory without a BC break. In terms of future direction that we could go is making it mandatory to pass Location from calling code (i.e. when constructing the ChoiceLoader in question). This would then lead to removal of LocationService dependency in ChoiceLoader itself - instead developers would use LocationService in calling code as necessary.

Summoning @alongosz for architecture design decision

@Steveb-p I'm not really following what happens here, but from the clean code POV if you were to introduce Location value so you can entirely drop dependency on LocationService and costly loading of Repository data, I would always support that.

That said, it of course depends on the required changes to the calling code. If this is used only within the scope we see in the PR - it should be fine. If it's used elsewhere, then depends on such diff as well.

src/lib/Form/Type/Content/Translation/TranslationAddType.php

sonarcloud · 2021-09-09T11:39:51Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

barw4 · 2021-09-15T13:59:35Z

Merged into master: c6b97ae

IBX-952: Added chosen Location to Translation Choice form

24d375e

barw4 added Bug Ready for review labels Sep 2, 2021

barw4 self-assigned this Sep 2, 2021

IBX-952: Fixed issues with null Location

961952b

barw4 requested a review from a team September 2, 2021 21:55

mikadamczyk requested review from mikadamczyk, adamwojs, Nattfarinn and webhdx September 3, 2021 06:44

webhdx approved these changes Sep 3, 2021

View reviewed changes

mikadamczyk reviewed Sep 3, 2021

View reviewed changes

ViniTou approved these changes Sep 3, 2021

View reviewed changes

mikadamczyk approved these changes Sep 3, 2021

View reviewed changes

Steveb-p requested changes Sep 3, 2021

View reviewed changes

barw4 requested a review from Steveb-p September 3, 2021 13:52

Steveb-p requested a review from alongosz September 3, 2021 14:22

alongosz reviewed Sep 3, 2021

View reviewed changes

GrabowskiM force-pushed the 2.3 branch 2 times, most recently from 12df4e9 to e436c4d Compare September 7, 2021 13:26

barw4 added Ready for QA and removed Ready for review labels Sep 9, 2021

Steveb-p requested changes Sep 9, 2021

View reviewed changes

src/lib/Form/Type/Content/Translation/TranslationAddType.php Outdated Show resolved Hide resolved

IBX-952: Applied review remark

1d785fa

Steveb-p approved these changes Sep 9, 2021

View reviewed changes

bogusez unassigned barw4 Sep 13, 2021

bogusez self-assigned this Sep 13, 2021

bogusez approved these changes Sep 15, 2021

View reviewed changes

bogusez added QA approved and removed Ready for QA labels Sep 15, 2021

barw4 added Ready for MERGE and removed QA approved labels Sep 15, 2021

mikadamczyk merged commit 5ab572d into 2.3 Sep 15, 2021

mikadamczyk deleted the ibx-952-fix-translation-list-loader branch September 15, 2021 13:51

barw4 mentioned this pull request Nov 3, 2021

IBX-1356: Backported fix for TranslationChoiceLoader #1996

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-952: Added chosen Location to Translation Choice form #1885

IBX-952: Added chosen Location to Translation Choice form #1885

barw4 commented Sep 2, 2021 •

edited

Loading

mikadamczyk left a comment

mikadamczyk Sep 3, 2021

webhdx Sep 3, 2021

mikadamczyk Sep 3, 2021 •

edited

Loading

webhdx Sep 3, 2021

Steveb-p Sep 3, 2021

mikadamczyk Sep 3, 2021 •

edited

Loading

mikadamczyk Sep 3, 2021

Steveb-p Sep 3, 2021 •

edited

Loading

ViniTou Sep 3, 2021

webhdx commented Sep 3, 2021 •

edited

Loading

Steveb-p left a comment

barw4 commented Sep 3, 2021

Steveb-p commented Sep 3, 2021 •

edited

Loading

alongosz left a comment

sonarcloud bot commented Sep 9, 2021

barw4 commented Sep 15, 2021

IBX-952: Added chosen Location to Translation Choice form #1885

IBX-952: Added chosen Location to Translation Choice form #1885

Conversation

barw4 commented Sep 2, 2021 • edited Loading

Checklist:

mikadamczyk left a comment

Choose a reason for hiding this comment

mikadamczyk Sep 3, 2021

Choose a reason for hiding this comment

webhdx Sep 3, 2021

Choose a reason for hiding this comment

mikadamczyk Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

webhdx Sep 3, 2021

Choose a reason for hiding this comment

Steveb-p Sep 3, 2021

Choose a reason for hiding this comment

mikadamczyk Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

mikadamczyk Sep 3, 2021

Choose a reason for hiding this comment

Steveb-p Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

ViniTou Sep 3, 2021

Choose a reason for hiding this comment

webhdx commented Sep 3, 2021 • edited Loading

Steveb-p left a comment

Choose a reason for hiding this comment

barw4 commented Sep 3, 2021

Steveb-p commented Sep 3, 2021 • edited Loading

alongosz left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 9, 2021

barw4 commented Sep 15, 2021

barw4 commented Sep 2, 2021 •

edited

Loading

mikadamczyk Sep 3, 2021 •

edited

Loading

mikadamczyk Sep 3, 2021 •

edited

Loading

Steveb-p Sep 3, 2021 •

edited

Loading

webhdx commented Sep 3, 2021 •

edited

Loading

Steveb-p commented Sep 3, 2021 •

edited

Loading