Problem/Motivation

\Drupal\views\ViewExecutable::setDisplay() includes a helpful debug() statement if you try to set an invalid display.
However, this can be shown to end-users if a site is misconfigured.

What's more, when format_string() was converted to result in an object, the debug statement now shows it as an object!

Debug:
Drupal\Component\Render\FormattableMarkup Object
(
    [arguments:protected] => Array
        (
            [@display] => block_1
        )

    [string] => setDisplay() called with invalid display ID "@display".
)
in Drupal\views\ViewExecutable->setDisplay() (line 792 of core/modules/views/src/ViewExecutable.php).

Proposed resolution

Either fix the debug statement, or remove it.

Remaining tasks

Credit http://drupal.org/u/tinny (who was the one who found this)

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Issue summary: View changes

Mentioning that tinny was the one who found this.

shashikant_chauhan’s picture

Lendude’s picture

Status: Active » Needs review
FileSize
1.28 KB

I would say, take it out. Printing debug information on a live system feels wrong. Looks like Views is the only place in core where this currently happens outside of tests, so should we maybe take them all out?

Other spots I see:
\Drupal\views\Plugin\views\field\FieldPluginBase::addAdditionalFields
\Drupal\views\Plugin\views\style\Opml::render
\Drupal\views\Plugin\views\style\Rss::render
\Drupal\views\Plugin\views\style\StylePluginBase::render

Patch is just for taking out the one in \Drupal\views\ViewExecutable::setDisplay()

dawehner’s picture

+++ b/core/modules/views/src/ViewExecutable.php
@@ -788,7 +788,6 @@ public function setDisplay($display_id = NULL) {
-      debug(format_string('setDisplay() called with invalid display ID "@display".', array('@display' => $display_id)));

+++ b/core/modules/views/tests/src/Kernel/ViewExecutableTest.php
@@ -195,8 +195,6 @@ public function testSetDisplayWithInvalidDisplay() {
-    // Error is triggered while calling the wrong display.
-    $this->setExpectedException(\PHPUnit_Framework_Error::class);

Can we maybe use a trigger_error or at least some error logging instead?

edwardchiapet’s picture

I agree that some sort of error logging would be more helpful than suppressing/not showing the error at all.

dawehner’s picture

Status: Needs review » Needs work

Needs work based upon #6 and #5

vaplas’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
1.89 KB

I do not know which approach is better. On the one hand, the desire to remove this noise is understandable. On the other hand, if the default display has a similar appearance, then the mistake can really not be noticed. Because default display shows instead of invalid display. In addition, trigger_error() has compatible behavior with debug().

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Well definitely v2 for me.

Just thinking about this, and why do we tolerate an invalid ID in the first place? Fail early, fail hard. This is not something that should ever come up in normal use/through the UI, so it's people using bad code, so lets make sure it fails.

This might be to much of an API change, but throwing an exception makes more sense to me. Lets see if this makes anything else fail.

dawehner’s picture

This code is probably coming from Drupal 6, where exceptions weren't really a thing, given views was compatible with php 4.

On the other hand, its a bit tough for me to judge, whether its okay or not to potentially break sites like that. Currently they render the default display, don't they? I think using trigger_error for a while would be certainly the more safe option.

Status: Needs review » Needs work

The last submitted patch, 10: 2853359-10.patch, failed testing. View results

vaplas’s picture

Issue tags: +blocker

#10: +1. Exception looks like fair behavior.

But also in Drupal there are other places for which the Exception also makes sense to discuss, but now use a softer behavior too:

So the discussion of correct behavior can be delayed. Therefore, to fix the problem here only with the output of the error - it seems logical.
Hence #11: +1 too :)

And like follow-up we can open issue to think about behavior and other cases per#4.

Lendude’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs work » Reviewed & tested by the community
FileSize
1.68 KB

Yeah, from a BC standpoint the exception is too much for now, just wanted to put it out into the world.

Just re-upping @vaplas' patch from #8 to make the last patch in the issue the one we are discussing. I think we all agree that trigger_error is the soft but right way to go here right? So RTBC then.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: v2_2853359-8.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.89 KB
+++ b/core/modules/views/src/ViewExecutable.php
@@ -2,6 +2,7 @@
+use Drupal\Component\Render\FormattableMarkup;

One line ran away :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2853359-16.patch, failed testing. View results

vaplas’s picture

Sorry, Drupal ran too. Now I understand #14.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Bot happy, we happy.

Cottser credited tinny.

Cottser’s picture

The exception vs. error decision is not one I'm comfortable making but the patch looks great to me. Adding credit for @tinny as mentioned in the OP.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2853359-18.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Reviewed & tested by the community