Content Translation module still implements hook_enable

Patch looks good, but we need some test coverage for this. Just checking whether messages are displayed.

@plach should this test be for enabling/disabling modules with messages in the hook or just this specific module has that message after install?

just this specific module has that message after install

:)

Re-rolled the previous patch and added test.

Marked #2632560: Content translation implements nonexistent hook_enable as duplicate.

Needs work given it fails.

Just wonder if this empty patch will fail.

HelpEmptyPageTest::testEmptyHookHelp() should only check hook_help() but not module installation process.

Minor cleanup.

+++ b/core/modules/content_translation/content_translation.install
@@ -5,27 +5,28 @@
-}
...
-/**
- * Implements hook_enable().
- */
-function content_translation_enable() {

Discussed in #2632560-5: Content translation implements nonexistent hook_enable, we want to keep the function. So kinda like this:

index d44b2d1..11b30c5 100644
--- a/core/modules/content_translation/content_translation.install
+++ b/core/modules/content_translation/content_translation.install
@@ -12,10 +12,14 @@ function content_translation_install() {
   // Assign a fairly low weight to ensure our implementation of
   // hook_module_implements_alter() is run among the last ones.
   module_set_weight('content_translation', 10);
+  content_translation_enable();
 }

 /**
- * Implements hook_enable().
+ * Callback to provide warning messages on install.
+ *
+ * This is hook_enable() of Drupal < 8 versions. Left here to avoid fatal for
+ * direct calls from contrib modules.
  */
 function content_translation_enable() {
   // Translation works when at least two languages are added.

I marked content_translation_enable() as deprecated.

+++ b/core/modules/content_translation/content_translation.install
@@ -5,27 +5,38 @@
+ * because if Language module is enabled simultinuesly with Content translation

simultaneously

Fixed typo.

I will give this a try for the sprint weekend

Nothing to do actually, the patch and the test look OK to me.

Patch in #22 looks fine and applies to HEAD.

+++ b/core/modules/content_translation/content_translation.install
@@ -5,27 +5,38 @@
+ * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0. This is

So the problem here is that we are not just going to remove this method we're going have to move the functionality to hook_install. BUT I have to disagree that hook implementations are API - you are not meant to call hooks directly and if you do that is wrong and contrary to our API. Therefore we should be able to remove hook implementations in minor versions at least. Especially hook implementations that have no impact.

All of this said I'm really sad that we're not using routes here - it feels really wrong. If a module depends on another module it ought to be able to rely on its routes being there. This code will break if someone installs language and alters the routes to have different paths (for whatever reason).

I think modules that need to be able to add post installation messages need to have a way to do so that is route safe. As this means a module can't even rely on its own routes.

See #2589967: Rebuild routes immediately when modules are installed and #2572293: Race condition triggerable by a single user due to router rebuild in kernel.terminate for some issues on that topic. We can add a @todo for those and change it back?

Yeah an @todo would help - probably pointing to #2589967: Rebuild routes immediately when modules are installed.

The patch is the same as #16 but with different comments.

Looks good.

+++ b/core/modules/content_translation/content_translation.install
@@ -12,20 +14,16 @@ function content_translation_install() {
     $message = t('Be sure to <a href=":language_url">add at least two languages</a> to translate content.', $t_args);

I know this isn't touched by the patch, but the string looks wrong to me. Sites will have one language installed, so they only need to add at least one language, not two.

Since this message isn't showing at all at the moment, seems like something we could change here. Or if not, let's open a follow-up.

For the string something like:

This site has only a single language enabled. Add at least one more language in order to translate content.

I am not sure how it works before, but toUriString is not suitable here.

Updated text in the test.

We might want to assert URLs in those messages.

Changing the translated string means that we can only do this in 8.1.x. I think we should make this change without the string change because then we can fix this in 8.0.x and 8.1.x and open a followup to change the message. Thanks for all the patches @Chi.

Back to needs work for #45

created follow up for string update: #2662078: Update the misleading warning message in Content Translation module

Looks good, thanks for the fixes.

No longer applies to 8.1.x now that #2662078: Update the misleading warning message in Content Translation module has been committed, so needs a quick 8.1.x re-roll.

Quick reroll...

Changing the branch and retesting...

Yay, thanks!

New test assertion needed to change with that string change too. Here's the fix, tested locally against that fail.

ah right. I missed that why we had to re-roll. thanks @joelpittet. Now it's RTBC as per #55

+++ b/core/modules/help/src/Tests/HelpEmptyPageTest.php
@@ -7,7 +7,6 @@
-use Drupal\Core\DependencyInjection\ContainerBuilder;

@@ -34,15 +33,6 @@ protected function setUp() {
   /**
-   * {@inheritdoc}
-   */
-  public function containerBuild(ContainerBuilder $container) {
-    parent::containerBuild($container);
-
-    $container->set('url_generator', new SupernovaGenerator());
-  }
...
   public function testEmptyHookHelp() {

@@ -59,6 +49,10 @@ public function testEmptyHookHelp() {
+
+    // Enable URL generator which always throws an exception.
+    \Drupal::getContainer()->set('url_generator', new SupernovaGenerator());
+

I think this change is wrong. In reality the container should be frozen after the containerBuild is called.

Given this is a kernel test there is no need to call module install here. We can replace the module install with...

    $this->enableModules(array_keys($all_modules));
    $this->installEntitySchema('menu_link_content');

We should also have a follow up to improve the HelpEmptyPageTest so that it tests that if a route is passed in to hook_help that the supernova generator does indeed throw an error.

$this->installEntitySchema('menu_link_content');

It's not clear what is so special with menu_link_content schema. Looks as some another trick at least if it is not commented.

@chi: can you post an interdiff to help review?

Notice that patch in #58 is outdated because of #2338747: Move {router} out of system.install and create the table lazy.

@chi: instead of an interdiff, can you roll an updated patch please?

@Gábor Hojtsy, patch in #62 is up to date. It's made per #60 recommendations. I can also roll patch from #58 if needed.

Yeah sorry for the misunderstanding. The fix looks fine to me, thanks for the update!

@Chi it is not what is special with the menu_link_content schema - it is was is special about installing modules in a kernel test base. The module installer shouldn't be used for this and the class has it's own methods to enable modules in its funky way. We need to install the menu_link_content schema because that test depends on it.

Committed 9ba7422 and pushed to 8.1.x. Thanks!

In addition to crediting those that provided a patch I've created @Gábor Hojtsy and myself for providing reviews and @catch for the suggestion that lead to #2662078: Update the misleading warning message in Content Translation module.

Given that nothing is actually broken I would hold off on porting this to 8.0.x - if people think I'm wrong please re-open the issue.

Yay, thanks! I think its fine on the 8.1 branch, it is soon released :)