Problem/Motivation

Currently ContentTranslationController assumes all content entity types have a 'default' form handler, which is in no way standardized or documented. A good example of where it breaks is #2833054: InvalidPluginDefinitionException: The "group" entity type did not specify a "default" form class..

Proposed resolution

Make ContentTranslationController behave like DefaultHtmlRouteProvider and check for an 'add' or 'edit' form handler
This would be a short term fix with no BC break until we get a full solution in #2006348: Remove default/fallback entity form operation

Remaining tasks

User interface changes

API changes

Data model changes

Comments

kristiaanvandeneynde created an issue. See original summary.

tim.plunkett’s picture

Category: Feature request » Bug report
Issue summary: View changes
Related issues: +#2006348: Remove default/fallback entity form operation

I'd consider this a bug because there is nothing to indicate that 'default' is required.

Berdir’s picture

I don't think we need to check for add, edit should be enough?

kristiaanvandeneynde’s picture

Status: Active » Needs review
FileSize
2.6 KB

This is merely a proof of concept as I want to take a look at ContentTranslationRouteSubscriber and see whether we can't do this cleaner. Setting to needs review to see how current tests deal with it.

I mean: The ::add() and ::edit() method state that $entity_type_id is optional, but it doesn't seem to be. It's always provided coming from the route subscriber and ::add() would arguably crash if it wasn't provided because it calls :;prepareTranslation() using the optional route parameter as a mandatory argument.

kristiaanvandeneynde’s picture

Whoops, bad copy-paste is bad. (Changed one 'add' to 'edit').

kristiaanvandeneynde’s picture

Re #3: That's the thing, there's no convention. I'd much rather check for both 'add' and 'edit' like DefaultHtmlRouteProvider does now than assume everyone uses the same form for creating and editing entities. It's not a perfect solution, but at least it would cater to more modules until we decide what we want to do about the real issue behind this.

The last submitted patch, 4: drupal-2848120-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: drupal-2848120-5.patch, failed testing.

tim.plunkett’s picture

Version: 8.3.x-dev » 8.4.x-dev
FileSize
3.1 KB
2.44 KB

Note that issues like this should be targeted against 8.4.x, but can be considered for backport once they are committed to that branch. See the updated alpha release policy. Thanks!

(copy-pasted that from other issues being updated)


content_translation_form_alter() says

Let the content translation handler alter the content entity edit form.

But there is test coverage ensuring it works for the 'add' form as well.

Also I fixed the previous change to keep the original comment.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review

Okay thanks for the input. Let's see what testbot says?

tstoeckler’s picture

This looks good to go for me, it is consistent with DefaultHtmlRouteProvider. Thank you for this patch, this is currently quite an annoyance for many people. I fear that this won't go by the committers without tests, but not sure.

tstoeckler’s picture

+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -338,6 +338,10 @@ public function add(LanguageInterface $source, LanguageInterface $target, RouteM
     //   $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
     //   See https://www.drupal.org/node/2006348.

@@ -369,6 +373,10 @@ public function edit(LanguageInterface $language, RouteMatchInterface $route_mat
     //   $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
     //   See https://www.drupal.org/node/2006348.

Maybe we want to remove the @todo's now? The todos kind of go in a different direction, but I actually think this is preferable to the concept of a configurable default operation. Not sure what others thing.

kristiaanvandeneynde’s picture

Small nitpick:
+ // Use the add form handler, if available, otherwise default.
Is used for the edit one too, should say 'edit' there.

Re #12: I'd leave at least some form of @todo in to mark that this isn't a perfect fix. We still need to find a way to deal with all these assumptions surrounding form handlers. Do we want to have a 'default' form? Do we want to use a 'default_operation' entity type property? Or do we want no 'default' at all and make everyone have to choose a form handler explicitly?

The latter would make the most sense but is probably a BC break.

tstoeckler’s picture

Yes, I think the latter is the best option. Usually you know what you want to do with the entity, specifically if you are editing or adding it. The two usages in Content Translation are perfect example of that, the controller methods are literally called ::add() and ::edit(). So - as you note - we have to leave default for BC, but to me personally this is a perfectly acceptable long term fix to simply explicitly specify the wanted operation. Ideally Content Translation would check for the form operations during route building time and only add the add and edit routes if the appropriate form operation is available. That is also something that we can't due to BC, though. But that's why I would be in favor of removing the @todo, but I don't feel strongly about it.

kristiaanvandeneynde’s picture

Does the comment in #9 mean this has coverage already? If so I'm willing to set this to RTBC. I'm even re the @todo by the way, we can always revisit it when deciding on what to do with the 'default' key.

kristiaanvandeneynde’s picture

Status: Needs review » Reviewed & tested by the community

Paging dr. Tim Plunkett :)

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/content_translation/content_translation.module
@@ -297,7 +297,7 @@ function content_translation_form_alter(array &$form, FormStateInterface $form_s
   // Let the content translation handler alter the content entity edit form.

Let's update this comment so it matches the code.

kristiaanvandeneynde’s picture

jonathanshaw’s picture

The comment in #18 sounds like the patch in #18 is intended to address @catch's comment from #17, but it doesn't.

kristiaanvandeneynde’s picture

*facepalm* My bad. I did fix another incorrect comment, though :) What should it say then? // Let the content translation handler alter the content entity add or edit form.?

jonathanshaw’s picture

That's what I reckoned, except shouldn't that be "forms" plural?

Pavan B S’s picture

Rerolled the patch, please review

tstoeckler’s picture

Status: Needs review » Needs work
Issue tags: +drupaldevdays

Thanks, the reroll looks good. Still needs work for #17.

tstoeckler’s picture

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
Issue tags: -DevDaysSeville
FileSize
3.36 KB
  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -297,7 +297,7 @@ function content_translation_form_alter(array &$form, FormStateInterface $form_s
    +  if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1 && ($op == 'edit' || $op == 'add' || $op=='default')) {
    

    When re-rolling, please make sure you re-apply the code 1:1. There's a coding standards violation in the re-rolled version: $op=='default'

Attached is a patch which re-rolls the patch from #18, updates the comment as requested by #17, changes the triple equation to an in_array() call and uses a ternary operator instead of an if-statement.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the re-roll.

Re-RTBCing per #16.

One minor note:

+++ b/core/modules/content_translation/content_translation.module
@@ -296,8 +296,10 @@ function content_translation_form_alter(array &$form, FormStateInterface $form_s
+  if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1 && in_array($op, ['edit', 'add', 'default'])) {

This should not hold this patch in particular, but just as a general note it's best practice to always use the third parameter of in_array() so that PHP does a strict comparison. With this the check would pass if $op is int(1), for example. (Again, in this particular case it's not a problem, because passing an integer as $op would be a bug somewhere else, but by using $strict = TRUE you can avoid people having to think about this stuff.)

kristiaanvandeneynde’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for the review Tobias. And you're right, strict comparisons are the way to go, especially in PHP :)

kristiaanvandeneynde’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -296,8 +296,10 @@ function content_translation_form_alter(array &$form, FormStateInterface $form_s
    +  if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1 && in_array($op, ['edit', 'add', 'default'])) {
    

    in_array() should pretty much always have the third argument and be strict about types.

  2. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -337,7 +337,9 @@ public function add(LanguageInterface $source, LanguageInterface $target, RouteM
    +    $operation = $entity->getEntityType()->getFormClass('add') ? 'add' : 'default';
    
    @@ -368,7 +370,9 @@ public function edit(LanguageInterface $language, RouteMatchInterface $route_mat
    +    $operation = $entity->getEntityType()->getFormClass('edit') ? 'edit' : 'default';
    

    I think we should use (for example) ->hasHandlerClass('form', 'add')

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
3.39 KB
2.85 KB

Here we go, if tests go green I'll set back to RTBC as nothing has changed conceptually.

kristiaanvandeneynde’s picture

Status: Needs review » Reviewed & tested by the community

  • catch committed 3cb5c72 on 8.4.x
    Issue #2848120 by kristiaanvandeneynde, tim.plunkett, Pavan B S,...

  • catch committed 0eda6a9 on 8.3.x
    Issue #2848120 by kristiaanvandeneynde, tim.plunkett, Pavan B S,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x, thanks!