The current selection strategy for a content renderer is based on the content entity type and then falls back to the group. It's a magic service key which isn't a very good API to begin with and forces modules that don't depend on entity_print to implement a ServiceProvider when they optionally want to provide a new print renderer.

Lets move this over to a entity handler.

CommentFileSizeAuthor
#10 interdif.txt1.04 KBbenjy
#10 2857785-10.patch10.51 KBbenjy
#8 interdiff.txt3.13 KBbenjy
#8 2857785-8.patch10.47 KBbenjy
#5 interdiff.txt9.12 KBbenjy
#5 2857785-5.patch10.24 KBbenjy
#3 2857785-3.patch3.18 KBbenjy

Comments

benjy created an issue. See original summary.

sumanthkumarc’s picture

sumanthkumarc commented

@benjy, Any progress on this?

benjy’s picture

benjy commented
Status: Active » Needs review
StatusFileSize
new 2857785-3.patch3.18 KB

Here's a first pass.

Status: Needs review » Needs work

The last submitted patch, 3: 2857785-3.patch, failed testing.

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2857785-5.patch10.24 KB
new interdiff.txt9.12 KB

Fixed a couple of tests and changed to the same approach for the main module.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

+1 I think this looks great compared to services, and for ease of integrating in other contrib (#2857785: Add a print entity handler)

sam152’s picture

sam152 commented

Looking good. Comments as follows.

  1. +++ b/entity_print.module
@@ -28,6 +29,17 @@ function entity_print_theme($existing, $type, $theme, $path) {
+    if ($entity_type instanceof ContentEntityTypeInterface && !$entity_type->hasHandlerClass('entity_print')) {

    You can use \Drupal\Core\Entity\EntityTypeInterface::entityClassImplements here. 

  2. +++ b/entity_print.module
@@ -28,6 +29,17 @@ function entity_print_theme($existing, $type, $theme, $path) {
+      $entity_type->setHandlerClass('entity_print', 'Drupal\entity_print\Renderer\ContentEntityRenderer');

    I prefer the concrete ContentEntityRenderer::class syntax for this too. 

  3. +++ b/modules/entity_print_views/entity_print_views.module
@@ -21,3 +21,12 @@ function entity_print_views_data_alter(array &$data) {
+  if (isset($entity_types['view'])) {

    If you have a dependency on views, this is probably not needed. 

  4. +++ b/src/Renderer/RendererBase.php
@@ -54,6 +56,18 @@ abstract class RendererBase implements RendererInterface {
+  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
+    return new static (
+      $container->get('renderer'),

    Most implementations I've seen of this pass $entity_type as the first param and store it on a property. Not sure if that might be useful down the track. 

  5. +++ b/src/Renderer/RendererFactory.php
@@ -17,23 +17,15 @@ class RendererFactory implements RendererFactoryInterface {
+    $entityManager = $this->container->get('entity_type.manager');

    technically this is the entity_type_manager. Also no camels for local variables.

benjy’s picture

benjy commented
StatusFileSize
new 2857785-8.patch10.47 KB
new interdiff.txt3.13 KB

Thanks for the review. All fixed apart from the entityType param.

Status: Needs review » Needs work

The last submitted patch, 8: 2857785-8.patch, failed testing.

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2857785-10.patch10.51 KB
new interdif.txt1.04 KB

Status: Needs review » Needs work

The last submitted patch, 10: 2857785-10.patch, failed testing.

  • benjy committed 7877062 on 8.x-2.x 
    Issue #2857785 by benjy, Sam152: Add a print entity handler
benjy’s picture

benjy commented
Status: Needs work » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.