Inform users/developers about entity/field mismatch details if it happens

The main thing we need to figure out here is whether it's acceptable to list mismatches in status report item itself or whether we need a separate page linked also from the status report item. We should also discuss the textual content of the status report error message.

Here is a very rough example of how things could look like:

Yes the example being here looks OK to me too. It's going to be shown to end-users very rarely, and if it's large then something very wrong is happening.

Wait, but what the hell is a mismatch ? :D

Is 'discrepancy' better?

Mismatch does sound a bit innocent and harmless, like odd socks to be honest.

@Bojhan:

What I was trying to convey is that the system expects some values and other were found in the installed code. There's nothing a site builder can do to fix this, aside from replacing the offending modules with working versions (either newer or older), but the message should be useful to identify which modules caused this problem to arise. Unfortunately we don't know exactly which modules are responsible for the mismatch, so we dump all we have in the hope the user will be able to figure it out.

I think what I would want to understand is if my site is now broken or not. Listing "mismatches" only gives me more detail about a general state I can't estimate the severity of. Not saying we should not do this, but needs some context on what this means.

Also, in the list as shown in the screen shot above, all those verbs (Create the field X, Delete the field Y), is that saying what happened, what should happen or what to do to fix things, or something else?

At the moment this error is pretty bad, it's the same kind of severity of pending database updates. For instance you may try to save a node and get an exception because data for a newly added field could not be saved, due to a missing column in the node table(s).

#2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code has the potential to mitigate it, but still this is something that should be fixed as soon as possible.

About the wording, the idea is that it's saying what is supposed to happen to fix things, however that's a legacy of when we applied these changes automatically via update.php (see #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state), we can definitely change that text to make it more suitable for this new scenario.

I think what I would want to understand is if my site is now broken or not. Listing "mismatches" only gives me more detail about a general state I can't estimate the severity of.

The list of mismatches is supposed to help site builders figure out what modules are responsible for the problem. Additionally it can serve as a developer tool, so one has less chances to forget about writing the proper update functions reconciling the system information with the installed code.

At my organization, we've hit this same message on two different Drupal 8 projects as a result of upgrading to the latest beta release. At a minimum, having a raw output would be very helpful because we have no real understanding of the magnitude of the issue and whether we should simply revert an entire section of the site OR work through at a programmatic level to see if we can resolve the issue.

I know this is only set to major, not critical (and yes I realize this error is an edge case that doesn't make it worthy of a bump to critical). However, it is a blocker to site builders that really can't make an informed decision without some additional insight. Therefore, either solution (on status report or as a separate page) would be well received. Given the potential volume of output, a separate page would probably be the best long term solution.

This received enough reviews.

This received enough reviews.

Agree with @rickmanelius -- we've also encountered this error as a result of upgrading through the betas, and some information on at least what modules are at issue would really, really help. Even more details would be great, but at the very least it would be nice to go from a troubling message with zero help to a troubling message with some leads to follow.

Yeah +1 to some helpful errors, I had to figure this out with xdebug this week. I also found a relevant question on Stackoverflow where someone had been stuck with this for a week.

Is this still eligible for RC? If so, I might take a crack at doing this later today.

I think it depends a lot on what the patch does. If all it does is expose data or logs that exist somewhere to the frontend, then I'd say it's basically an improvement of user-facing documentation and therefore rc eligible. If it does a bunch of other stuff, then maybe not.

In case this opaqueness doesn't get addressed right away, can you point to the Stackoverflow issue where they fixed this? Because I'm guessing this page is where a lot of developers are going to land when they encounter this issue for now.

I was just going to improve the status page exactly like the design in #2?

Here's the question: http://drupal.stackexchange.com/questions/176266/is-it-a-problem-when-en...

The change record is the best source of info now, I linked the question there.

Attached is a first attempt at this, using getChangeSummary() which gives us a bit of an idea of what needs doing.

And here's a screenshot with a few more errors.

I've tried applying this patch to RC3 and HEAD and it doesn't apply anymore.

Ekes did this quick reroll of this patch on my computer, so I'm posting it for him.

+1 for RTBC on this, I think it's a good improvement and something that is worth committing before release.

I think we should adjust the messages, as they are confusing now. Instead we should say something like:

The File type field needs to be installed

The Custom menu link entity type needs to be updated

Other than that I'd be fine with this.

I don't mind whether we put the action first or last, will wait see what others thing before re-rolling.

Are you suggesting we use the word "installed" over "created" as well?

The way messages are framed right now seem to imply an action needs to be taken by the site builder, I was trying to change the sentences to convey the idea that an action needs to be taken but it's not something the site builder can do.

Re "installed" vs "created", (un)install is the terminology the API uses (since field definitions already appeared in the system, i.e. they were created, but they were not installed properly), so I thought this would be more precise, whereas "create" can be confused with a UI-driven action.

My 2 cents :)

I'll work on this one tonight.

@penyaskito:

Are you still planning to work on this?

Sorry for the delay. Changed the messages as plach suggested on #26.

Before:

After:

Looks great to me!

+++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
@@ -74,15 +74,15 @@ public function getChangeSummary() {
+              $summary[$entity_type_id][] = $this->t('The %field_name field needs to be installed.', array('%field_name' => $storage_definitions[$field_name]->getLabel()));
...
+              $summary[$entity_type_id][] = $this->t('The %field_name field needs to be updated.', array('%field_name' => $storage_definitions[$field_name]->getLabel()));
...
+              $summary[$entity_type_id][] = $this->t('The %field_name field needs to be deleted .', array('%field_name' => $original_storage_definitions[$field_name]->getLabel()));

Minor (can be fixed if we happen to reroll this): can we use the [] array syntax here?

Fixed tests and changed arrays to short syntax in all the changed lines.

Now for reals

Added screenshots to the IS

I just realized deletion messages are still talking about "delete" instead of "uninstall".

Oops, fixed that.

Forgot to rebase.

Hide files so testbot ignores them.

Updated screenshot

Hopefully this will stick now ;)

There is a reference to this issue in a comment in system.install: https://www.drupal.org/node/2554911
Can we remove that on commit? I'll try to reroll later, but in case it's committed before I get to it.

Yep, that should be removed, thanks.

Removed that reference. Leaving as RTBC.

Since this contains numerous improvements for translatable strings, we should commit it to 8.1.x per https://www.drupal.org/core/d8-allowed-changes#minor.

I think the last hunk by itself, which adds a string and additional information to an admin-facing error message, might be able to go into 8.0.x once we commit the whole thing to 8.1.x. Worth considering, in any case.

IMHO this really happens quite often (at least on all the sites I moved during the beta phases), so adding to 8.0.x should be preferred, IMHO. Its damn hard to figure it out otherwise.

+1 on back-porting to 8.0.x the non disruptive hunk.

It's going to be shown to end-users very rarely

Maybe more common than you may expect.

We are running a clean Drupal 8.0.0 install, migrated from a D6 site, with Drupal Upgrade. No other contrib themes / modules. Migrate worked fine, no warnings in the status report.

Then 1 hour into fixing missing non-migrated fields / views and we hit this notice. It is very baffling for me (as a developer) and would be even more so for a normal user...

this really happens quite often (at least on all the sites I moved during the beta phases)

I can confirm what @dawehner says above as I am seeing this on a site I moved from RC3 with very minimal complexity content types. I have no idea where it's coming from. I imagine quite a few (D8 beta / RC) sites were being run in production and will come across this.

Content types

Page (default)
Blog (custom) :
Body body - Text (formatted, long, with summary)
Category - field_category - Entity reference
Image - field_image - Image
Tags - field_tags - Entity reference
Gallery (custom) :
Body body - Text (formatted, long, with summary)
Category - field_category - Entity reference
Image - field_image - Image
Images - field_images - Image

FYI there's also a drush entup command that can help you with this meanwhile. If you wish to learn more on this, see the last 10 minutes or so of https://events.drupal.org/barcelona2015/sessions/entity-storage-drupal-8....

@plach, thank you for this info! It sorted out the warning by updating the following:

drush entup
The following updates are pending:

node entity type :
  Update the node.field_image field.
  Update the node.field_tags field.

If this patch is too risky to include, then would a patch with a simple link pointing to this issue or a CR stating the "drush entup" detail, etc. be sufficient to include in 8.0? It's a fairly mysterious error to be including with no associated info on a reports page IMO.

Blindly executing drush entup is not recommended in general because on a non-upgraded D8 installation you should not get these errors unless the site code base is doing something wrong. And in that case applying drush entup may just "hide the dust under the rug". However if you know why there's a mismatch, for instance you wrote some custom code, drush entup is a perfect developer tool. Otherwise you need to write an update function to reconcile the entity/field definitions as explained in the change records:

https://www.drupal.org/node/2554097
https://www.drupal.org/node/2554101

@plach - thanks for clarification. [Perhaps a patch that provides link(s) to these CRs then on error :-/]

@mccrodp this change isn't risky, but it will break translations (which are only just getting caught up with 8.0.0) which is why it's been moved to 8.1.x.

Like @xjm I think the last hunk of the patch is completely fine to put into 8.0.0.

I'm actually going to move this back to 8.0.x

Let's commit just that last hunk to both branches, then open a new issue to fix the translations in 8.1.x

That way we don't have to diver 8.0.x and 8.1.x too quickly, we can get the main improvement out in 8.0.1 hopefully.

CNW just because i'm pushed for time - this just needs the patch re-uploading without the string changes.

Here's just the last hunk. Moving back to RTBC.

+1 for splitting he string breaks into another patch. Thanks!

Oh also, I think in D7 we would mention string additions in the release notes, so we should do that for this issue too.

Hmm, I think we should try to do that with a change records link if we can. That would mean a change record for each issue with a string addition though, but means we can build the list of notable changes between releases over time.

I was comparing with https://www.drupal.org/node/1527558 which says "release notes mention" for an added or changed admin-facing string. Not sure about a change record -- normally we would not provide change records for strings.

For now I'm adding a "String change in 8.0.1" tag that we can check before we tag that release. If we want to do a CR or something else for these, we can go over issues with that tag. I'll also check with the l.d.o maintainer for feedback or advice.

Meanwhile, catch, alexpott and I all agreed on the last hunk going into 8.0.x because the impact of this bug is much more problematic than an string that will not be translated in some cases. Thanks to everyone who helped confirm the impact of this bug on real sites as well. Committed and pushed to 8.1.x and 8.0.x. We still need the followup to put the other string improvements in a separate patch.

Also created #2623232: Improve messages when entity/field mismatch happens. If you helped write those strings in this issue, you can comment over there so we can add issue credit for you.