Flood of warnings after updating to Diff 8.x-1.0-alpha5

Thx for reporting.

I see from checking the code,
- these keys are not defined as APIs or really guaranteed on return values although expected
- we rely on them

Something is completely wrong with our annotation where
\Drupal\diff\DiffEntityComparison::getRows
indicating to return array on annotation, but returns a call indicating string
\Drupal\Component\Diff\DiffFormatter::format

I guess we need to check the doc header annotation and health check the return values.
And / or consider working more with exceptions to deal with unexpected situations.

Promoting for release consideration to clean our APIs.

Can this make the difference?

Also, the documentation of the individual methods should really document very cleanly what structure of array returns they guarantee.
And if they don't guarantee, we need to check before accessing.

Promoting this to critical for release.

We need to maintain our API boundaries and guarantees and use them well.

Actually, I could reproduce. Will test and upload a patch soon.

Thanks for reporting, the logs were growing exponentially! This should fix it. Also modified a bit some functions.

Oh btw, bug reports go always against the dev version.

Did you check applying the last patch? The markup will be there as long as we have a 'data' key. I've tested, since I was the same kind of warnings (both #markup and data) and I've seen the last patch fixes that.
I've seen the 3 and 1 as keys, that's why my code checks both, if not present just "continue". Please check with the last patch and report if it fixes the problem or not.

Thank you @dhendriks! Then this should be RTBC?

I we have no interface because we are dealing with array, we should write what keys exactly we are creating and returning.
A good example on how to do his is hook_requirements:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

And if it's not us defining it, we should refer to a different location with @see.

Also, you are trying to make us commit a workaround for an "edge case" that is not covered in tests. I guarantee this will break again because someone will clean up this code.
From the issue description above i have zero idea what causes the problem except #7 "Actually, I could reproduce. ".
So please try to provide a test about how to create this case or at least explain.

Actually is not an edge case, it was happening because I was trying to get info from places where is none.

I thought that this type of error throws notices that lead to test fails anyway.
It's new to me that this needs dblog module enabled.

+++ b/src/DiffEntityComparison.php
@@ -191,8 +191,11 @@ class DiffEntityComparison {
    * @return array
-   *   Array of rows usable with theme('table').
+   *   Array of rows usable with theme('table') returned by the core diff
+   *   formatter when format a diff.
    */

theme() doesn't exist anymore. Should say #type table or table render element as that's what we use, not the template directly.

And yes, any kind of PHP notice or warning should result in exceptions in the test. Not sure what's happening

Actually the tests can be much shorter. :)

Doesn't that imply that there are no tests for "Unified fields" right now? Shouldn't we add a bit more than just a assert 200?

hmmm actually this breaks when a new entity is present in the right side but not in the left side. Will check.

Actually the previous patch was breaking the case when an entity is in one revision and not in the other, this should fix it. A review of @dhendriks with the new patch can be also good.

can you give us an example about when you get warnings, how is the diff displayed? :) please

Tests unchanged. Actually it was obvious that it was going to throw an exception. We also needed to change in there. :) This time should be clean. :)

Thanks for reporting back. :)

+++ b/src/Plugin/diff/Layout/UnifiedFieldsDiffLayout.php
@@ -148,8 +148,8 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+        if (isset($field_diff_rows[$key][1]['data']) && trim($field_diff_rows[$key][1]['data']['#markup']) != '') {
+          if ($field_diff_rows[$key][1] == $field_diff_rows[$key][3]) {

@@ -174,8 +174,8 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+        if ($field_diff_rows[$key][1] != $field_diff_rows[$key][3]) {
+          if (isset($field_diff_rows[$key][3]['data']) && trim($field_diff_rows[$key][3]['data']['#markup']) != '') {

Why are these lines inversed in sequence? :-)

Committed, thx!

Because the first if is to define if we need to add 1 to the row count when there is a value to diff. the second one checks first if there are differences between the values, if so displays a row, otherwise it prevents duplicated lines. :)