Simplify data mapper with unlocalized and localized dimension content #205
Conversation
alexander-schranz
force-pushed
the
feature/simplify-data-mapper
branch
3 times, most recently
from
e5387c3 to
6b646ae
Compare
alexander-schranz
changed the title
alexander-schranz
force-pushed
the
feature/simplify-data-mapper
branch
4 times, most recently
from
d5f1c27 to
082fb0a
Compare
alexander-schranz
force-pushed
the
feature/simplify-data-mapper
branch
from
082fb0a to
fc1db96
Compare
alexander-schranz
force-pushed
the
feature/simplify-data-mapper
branch
from
fc1db96 to
5f6be67
Compare
| @@ -147,44 +135,43 @@ public function map( | |||
| } | |||
|
|
|||
| if (null === $entityClass || null === $routeSchema) { | |||
| // TODO FIXME add test case for this | |||
| return; // @codeCoverageIgnore | |||
| return; | |||
There was a problem hiding this comment.
i think this is most likely an error (eg forget to add configuration). i think i would throw an exception here too?
| @@ -136,6 +136,10 @@ public function postAction(Request $request): Response | |||
| { | |||
| $example = new Example(); | |||
|
|
|||
| $this->entityManager->persist($example); | |||
| // FIXME This flush is needed for RoutableMapper and should in future note longer be required | |||
There was a problem hiding this comment.
why was this not needed before? 🤔
There was a problem hiding this comment.
We did skip when no ResourceId was given, should we readd this?
There was a problem hiding this comment.
okay i see 🙂 i am not sure..
routes for pages are created only when they are published - maybe we should do that here too?
There was a problem hiding this comment.
but i dont have a strong opinion on this, we dont need to change it now
There was a problem hiding this comment.
You are definitly right we currently have bug in the RoutableDataMapper after the entity is once published. The draft route edit is directly editing without ever being published.
| $title = $this->getTitle(); | ||
|
|
||
| if ($title) { | ||
| $data['title'] = $title; |
There was a problem hiding this comment.
is there a reason for doing this? 🤔
i think the api for pages returns null values too
There was a problem hiding this comment.
Would make most tests confusing when its always returned here.
There was a problem hiding this comment.
Testcases using like Routablemapper using getTemplateData() but don't have and title in there mapping.
There was a problem hiding this comment.
I changed the handling here by writing the 'title' into the TemplateData in the setTemplateData and the only way to set the title is now setTemplateData.
There was a problem hiding this comment.
So getTemplateData will only return a title when it really did set a title and not magically return a title when never was set.
Co-authored-by: nnatter <niklas.natter@gmail.com>
alexander-schranz
force-pushed
the
feature/simplify-data-mapper
branch
from
f563e9e to
748ce86
Compare
| @@ -80,6 +80,10 @@ public function map( | |||
| throw new \RuntimeException('LocalizedDimensionContent needs to extend the TemplateInterface.'); | |||
| } | |||
|
|
|||
| if (DimensionContentInterface::STAGE_LIVE !== $localizedDimensionContent->getStage()) { | |||
| // return; | |||
There was a problem hiding this comment.
this condition does not do anything anymore - is okay for me to merge it like this as we decided to refactor this part of the code in a second pull request 🙂
For making the mapper easier to handle and avoid check for localized and unlocalized dimension I for changing the interface to the following way.
TODO: