// @todo Use a placeholder for the entity label if this is abstracted to
    // other entity types.

Regardless if it is or is not abstracted, using a placeholder means we don't have to hardwire in a constant, which means the code is more maintainable. So I think this should be done and the @todo removed.

CommentFileSizeAuthor
#9 3034254-9-remove-views-list-builder-todo.patch667 bytesTR
remove-views-list-builder-todo.patch997 bytesTR
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TR created an issue. See original summary.

TR’s picture

TR CreditAttribution: TR commented
Issue tags: +Novice
pratik.mehta19’s picture

pratik.mehta19 CreditAttribution: pratik.mehta19 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, remove-views-list-builder-todo.patch, failed testing. View results

TR’s picture

TR CreditAttribution: TR commented
Status: Needs work » Reviewed & tested by the community

The test fail in #4 was a temporary failure in the core quickedit module tests, unrelated to this patch.

That fail has now gone away, as can be seen by the retest and test history in the original post.

Resetting status.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -235,10 +235,12 @@ public function render() {
-    $list['enabled']['table']['#empty'] = $this->t('There are no enabled views.');

This same text can be be found in \Drupal\views_ui\Controller\ViewsUIController::reportPlugins - we should keep the translations aligned so translators have less strings.

TR’s picture

TR CreditAttribution: TR commented

ViewsUIController::reportPlugins() currently only looks at the plugin definitions and doesn't access the configuration entities, so pulling in the entities just to get the @label would be silly. I don't think that string should be changed.

@alexpott: So what's your suggestion? Should we just delete the @todo and leave the string as it is?

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented

I have looked at the previous issue where this todo was added [#183082] based on the assumption(see comment #38) that there would be the plural labels for the entity. However, we don't need singular in both texts. So shouldn't we just remove the @todo?

#7 +1 to it.

TR’s picture

TR CreditAttribution: TR commented
Priority: Normal » Minor
Status: Needs work » Needs review
FileSize
3034254-9-remove-views-list-builder-todo.patch667 bytes

OK, I read through that other issue. Here's a new patch just removing the @todo

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.
Marking RTBC based on the assumption that the @alexpott would have the same thoughts after #8

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Title: Fix for a @todo in ViewListBuilder » Remove @todo in ViewListBuilder
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed a1f4e9f043 to 8.8.x and c910d64ac9 to 8.7.x. Thanks!

As a documentation fix backported to 8.7.x

  • alexpott committed a1f4e9f on 8.8.x 
    Issue #3034254 by TR, alexpott, naveenvalecha: Remove @todo in...

  • alexpott committed c910d64 on 8.7.x 
    Issue #3034254 by TR, alexpott, naveenvalecha: Remove @todo in...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.