DDST-554: New roles added to Creator vs Contributor mapping #16
Conversation
|
|
||
| // Add the resolved image URL to the elements array. | ||
| $this->elements[$dest][] = $resolved_image_url; | ||
| } | ||
| } |
There was a problem hiding this comment.
So this was already setting a thumbnail?
... but it's doing an up-front lookup just to use the deferred thing? Should probably drop the up-front lookup to just always do the deferred thing?
Kinda want to suggest making use of the URL generator plugins, instead of directly using the underlying route.
| $url = $this->urlGenerator->generateFromRoute('dgi_image_discovery.deferred_resolution', [ | ||
| 'node' => $node_id, | ||
| 'style' => $style_id, | ||
| ], ['absolute' => TRUE]); |
There was a problem hiding this comment.
Not the intended URL Generator plugin (is this service even plugin based? Dunno)? Like, was meaning the plugins for the image discovery, in https://github.com/discoverygarden/dgi_image_discovery/tree/main/src/Plugin/dgi_image_discovery/url_generator
... so, using the related plugin manager and getting an instance of the desired plugin. Not sure if it's necessary here to make it fully configurable, but could? Not much configuration on either.
There was a problem hiding this comment.
@adam-vessey I made code changes, please review.
| @@ -389,11 +402,23 @@ public function addThumbnail(ContentEntityInterface $entity, $dest) { | |||
| $event = $this->imageDiscovery->getImage($entity); | |||
There was a problem hiding this comment.
This is still doing an unnecessary lookup. Why bother keeping this if doing the deferred resolution, which expects to handle the lookup separately?
|
Also, the latest commit introduced a number of coding standards issues: |
No description provided.