Sticky requirements error if HTML diff plugin unwanted

Discussed with miro.

1. If enabled and missing, show as error. If not enabled, make it a blue info message. "you could also use this awesome thing"

2. We need to make configuration on install dynamic as well. If it's not there, we must not enable it, so needs a hook_install() check, call getDefinition(), only then update config for it.

Hi Berdir

I have same error.

Diff Not found
The HTML Diff layout requires the HtmlDiffAdvanced library. Please consult README.txt for installation instructions.

What is your advice?

This should fix it. :)

Better to have it like this then?

This should be better.

What about now?

Hmm, what about this?

Let's see what is happening there.

Now should be better, this time with tests.

Sorry, my bad.

Drupal core 8.1.9
applied #22 patch on dev from 9/21, composer not installed.

After running update, on the Diff settings screen, Notices for both split_fields + unified_fields:

Undefined index: split_fields in Drupal\diff\Form\GeneralSettingsForm->buildForm() (line 95 of /PATH_TO/modules/diff/src/Form/GeneralSettingsForm.php).

(same for unified_fields)

These were unchecked in settings, and checking both made the notices go away. Once notices disappeared, the settings could be saved with these unchecked, but viewing a diff results in a Symfony\Component\Routing\Exception\InvalidParameterException.

Enabling both or just split fields seems to work. Enabling just unified_fields results in more errors I'm not going to track here now, hopefully this is helpful. Thanks for working on this.

Thank you @echoz but the problem you report is more related with #2801889: Parameter "filter" for route "diff.revisions_diff" must match... crash which has a patch uploaded that solves the issue, would be really helpful to test it and see if the problem you report is fixed. :)

Because that's how inmail fix the tests. :)

Since #2801889: Parameter "filter" for route "diff.revisions_diff" must match... crash was committed to dev 9/22, I patched it with your #22 patch that still applies, but just going to the Extend screen results in a white screen.

PHP Fatal error: Class 'Drupal\diff\DiffBreadcrumbBuilder' not found in /PATH_TO/core/lib/Drupal/Component/DependencyInjection/Container.php on line 296

Can you check clearing caches? There is not a DiffBreadcrumbBuilder since before alpha5. Don't think my change is related.

1. +++ b/diff.module
   @@ -92,16 +93,37 @@ function diff_help($route_name, RouteMatchInterface $route_match) {
   + *   An array of plugin and instance requirements.
   + */
   +function diff_get_requirements() {
   

   why the helper function? I don't think this is re-usable?

   I'd just keep it one function.

2. +++ b/diff.module
   @@ -144,19 +166,17 @@ function _diff_field_label($entity_type, $field_name) {
   +function diff_install() {
   +  $visual_inline = \Drupal::service('plugin.manager.diff.layout')->hasDefinition('visual_inline');
   +  // Disable Visual Inline layout if library not found.
   +  if (!$visual_inline) {
   

   wouldn't it be better to do the opposite, and enable if it is present?

   I'm not 100% sure but I think right now with no $phase check in hook_requirements(), the only reason you even get past the install time check is because the requirements hook is incorrectly defined in the .module file and not in .intall.

   Try moving it there and try to install it, I suspect it will then fail.

As discussed, this should be better. :)

Thanks @yongt9412, your comment in #29 helped me realize I should uninstall alpha4 first. Patched dev from 9/24 with patch from #31. Installed smoothly. the default settings work.

Choosing Unified OR setting Unified as the only layout does show the layout but with:

Warning: Illegal string offset 'data' in Drupal\diff\Plugin\diff\Layout\UnifiedFieldsDiffLayout->build() (line 177 of /PATH_TO/modules/diff/src/Plugin/diff/Layout/UnifiedFieldsDiffLayout.php).

Also repeated for line 178.

Setting Split fields as the only layout works.

and unchecking both should not be possible, when comparing, it results in:

Symfony\Component\Routing\Exception\InvalidParameterException: Parameter "filter" for route "diff.revisions_diff" must match "[^/]++" ("" given) to generate a corresponding URL. in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 193 of /PATH_TO/core/lib/Drupal/Core/Routing/UrlGenerator.php).

You do not need to uninstall and re-install, just clear the cache. But doesn't matter now.

Those problems have nothing to do with this issue. The first one has an existing issue, please create a new one if you can't find one about the second problem.

This looks fine to me now.

Hello everybody,
I am sorry to come back on this fixed bug, but for me, it is not fixed : I am kind of newbie and I don't know how to patch your patch file (the #22 AND the #31 or only the last one, #31 ?) in my website (not in local).

My website is still out of order. When I go to the update.php page, it's written : "Requirements problem" and I cannot go on.
Thanks for your ideas :-)

@lionel.aubert if you are using the last dev version the problem should be gone by now.

@yongt9412 that's it, thanks a lot !!!