Views should require a label, rather than falling back to an unhelpful ID

Per @lauriii's review at https://git.drupalcode.org/project/drupal/-/merge_requests/3690#note_200528, I reverted making views.view.*:label required from that merge request.

I think @lauriii is right that it'd be better to combine:

1. making the label required
2. deprecate the fallback logic in View::label() (+ trigger a deprecation error: )
3. updating the default views + tests

… all in a single issue: THIS issue. That'd definitely make it a more coherent change.

Updating the issue summary to make it clear which parts this MR already does vs does not yet do. Also linking issues.

The blocked has been committed.

Looking awesome!

Just 41 failures left, and only one real remark on the MR: the update path test coverage.

Once this is green, I think this only needs subsystem maintainer approval.

👍

No more remarks!

Only remaining blocker: subsystem maintainer review 😊

I'm not formally a subsystem maintainer for Views, but I used to co-maintain D7 (and earlier) Views, and am unofficially a quasi co-maintainer for core... 😅

Overall, +1 to this change. The MR looks really good. Nice to see upgrade path so folks don't have to worry about this, and upgrade path tests so we know it works. Opened one perfect-is-enemy-of-the-good style review thread about the post_update implementation. Feel free to ignore it if y'all think it's too edge-casey to worry about.

Other than that, I think this is RTBC.

Thanks!
-Derek

This is fine from a Views perspective. Sorry for the 100s of test views, seeing all those brings back memories :D

Good point on the behavior of \Drupal\views\Entity\View::label(). In that case, withdrawing my concern and moving to RTBC.

Thanks again,
-Derek

Sorry for the 100s of test views, seeing all those brings back memories :D

Sorry? 😅😁

Looks mostly good. Posted one comment regarding the upgrade path 😇

I was confused by the last commit, which didn't actually move the logic to hook_ENTITY_TYPE_presave().

Then I learned views_view_presave() calls ViewsConfigUpdater since #2989745: views_update_8500() inlines configuration changes instead of this being done on config save for bc, then it made sense 😄👍

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions.

I read the MR (not a review). Two things jumped out at me. One, the trigger error messages do not meet coding standards and 2) the new labels in the yml files are inconsistent some have single quotes and some don't. The first is a being fixed in #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard which is at RTBC and I would like to stop rerolling that. And the second also has an issue #2994928: Quote consistency in .info.yml files for Core modules that still requires work, but mostly because quotes aren't needed in yaml. I have rebased the MR and made changes for these two items.

Setting to NR so those changes can be reviewed.

Both changes make sense and look good,back to rtbc.

Saving issue credits.

This issue makes a lot of sense and the MR looks great. Pushed it to 11.x and published the CR.

The only thing that looked a little off to me was:

   public function label() {
     if (!$label = $this->get('label')) {
+      @trigger_error('Saving a view without an explicit label is deprecated in drupal:10.2.0 and will raise an error in drupal:11.0.0. See https://www.drupal.org/node/3381669', E_USER_DEPRECATED);

I feel like either that message should be moved to happen when the view is saved or the message text should be word-smithed to make a bit more sense in the context of it getting triggered here rather than during the save operation. Could someone open a followup for discussing that? Thanks!

This seems like something worth mentioning in the release notes as well.