Feature: ExtensionPoint PrototypeDetailsFactory #190
Conversation
enable it to override it like: Sitegeist\Monocle\Domain\PrototypeDetails\PrototypeDetailsFactoryInterface: className: Foo\Bar\MyFactory
| @@ -14,6 +14,7 @@ | |||
| */ | |||
|
|
|||
| use Neos\Flow\Annotations as Flow; | |||
| use Sitegeist\Monocle\Domain\PrototypeDetails\Props\PropValue; | |||
There was a problem hiding this comment.
was not imported - but used in doc comment
| /** | ||
| * @return UseCaseCollection | ||
| */ | ||
| public function getUseCases(): UseCaseCollection; |
There was a problem hiding this comment.
missing in interface
There was a problem hiding this comment.
Only if there is a need to add another collection implementation
There was a problem hiding this comment.
my phpstorm was just confused sometimes ^^
| 'PropValue must be a primitive, an array or an object ' . | ||
| 'of type \\stdClass. Got "%s" instead.', | ||
| is_object($value) ? get_class($value) : gettype($value) | ||
| sprintf( |
There was a problem hiding this comment.
missing sprintf
is_object($value) ? get_class($value) : gettype($value) was set as timestamp
There was a problem hiding this comment.
I do not like that the prototype details factory would be overwritten via objects.yaml for all prototypes. This will not allow to have the styleguide render objects that do not use presentationobjects. I would rather have this somehow triggered by the
PresentationObjects:PresentationObjectComponent
On the other hand this does not do harm and only adds an interface that allows to mess with it. So if there is no good way to get this coexisting i would probably still accept this.
|
hi ;) its planned that when one overrides the interface, it is ones responsibility to also inject the default details service and call it occasionally (when one cannot handle it to fallback to the default) more sophisticated would be a middleware like chain where we register it via the settings but i think this too complex for now (still fun though ^^) |
|
What speaks against configuring this on a per prototype base like many other things. Would be really flexible and still allow to set a default for all presentationObjects. |
|
Per prototype basis is to much work (for me as an integrator ^^) and while per prototype namespace would be working - it would not allow amixed approach per package (which is possible with presentation objects) i do agree that my proposed extension is not optimal, but it works and is consisting with the currently available monocle extensionpoint see the other service interface for providing the editor (i forgot the name) |
|
in a later point we also could make the fusion rendering easier/better extensible. current problem beeing that only one package can extend monocle per time - for advanced things we need a chain (maybe even provide it as optional Monocle.ExtensionChain package ;) ) if you like we can have a call about this soon ^^ |
You do not configure this per prototype as it is in the PresentatioObject:Component predefined |
|
aaah you mean because my prototype extends a certain |
Yes in there you can configure the detailsFactory for presentationobjects and no magic is needed at all. Especially no graceful sidestepping. For the monocle side in this pr that would mean to look into the fusionKey |
|
okay i am starting to get comfortable with the idea ^^ theoretical contra would be:
theoretical pro argument:
|
|
fyi some links i missed previously:
see fallback logic
i found the name ^^ it is the which also uses the fallback logic to moncole return $this->defaultPropsCollectionFactory->fromPrototypeForPrototypeDetails($prototype);
see how i did it currently ^^ (one way street - only allows either moncle default or the custom presentation/rendering) |
|
by doing this writeup i remembered another contra argument #190 (comment) the proposed interface extraction is only a unplanned extensibility point - since it could have been done purely architectually. but by introducing something as official as see here: https://github.com/sitegeist/Sitegeist.Monocle.PresentationObjects/blob/040e85037803f9808dcc51870661e38d68692478/Classes/Infrastructure/PrototypeDetailsFactory.php#L76-L92 only the last line is really different from the default - the rest is copy paste. my argument is: dont open up to much - there is only this one package in demand for now - and if the time comes craft a nice extensibility api which can controll everything: (tabs at the bottom / rendering / usecase providing / editor for props providing / etc...) currently the usecase flow is also a bit odd as we only pass on which usecase was selected to the preview instead its serialized props - that way we need to evaluate the usecases in the presentation rendering ... |
|
Adding an unplanned extension point with a fixed use case in place is a bit weird. I rather would like to have a clean api for that. It would be a no go for me to use presentationobjects at all if it would override monocle internals for all prototypes even if it would promise to behave well. If there were a proper api for that I might use po-s in important places. |
good point - id hate such promises too ^^ |
|
fyi i just found out that using
this extension point was introduced with https://github.com/sitegeist/Sitegeist.Monocle/releases/tag/v7.3.0 |
hmmm yeah would prefer a configuration there aswell ... however one could also say that in that case only adding the interface would be ok. If we already use this pattern we aswell use it twice. Will think about that and probably accept this pr then. |
enable it to override it like:
Extension point for sitegeist/Sitegeist.Monocle.PresentationObjects#1
...
Description is WIP will add some more details later