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
catch’s picture

Status: Reviewed & tested by the community » Needs review

Agreed on trigger_error() vs. introducing a new exception here, however do we want to make that an E_USER_WARNING instead of a notice, that might help to clean up existing incidences of the bug more so that we can introduce an exception in 9.0.0

Also - should we open a follow-up to introduce the exception?

vaplas’s picture

Status: Needs review » Needs work

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

vaplas’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
2.44 KB
  1. +++ b/core/modules/views/tests/src/Kernel/ViewExecutableTest.php
    @@ -198,8 +198,9 @@ public function testSetDisplayWithInvalidDisplay() {
         // Error is triggered while calling the wrong display.
         try {
           $view->setDisplay('invalid');
    +      $this->fail('Expected error, when setDisplay() called with invalid display ID');
         }
    -    catch (\PHPUnit_Framework_Error $e) {
    +    catch (\PHPUnit_Framework_Error_Warning $e) {
           $this->assertEquals('setDisplay() called with invalid display ID "invalid".', $e->getMessage());
         }
    

    I forgot to drop the test, in case of incorrect behavior. And clarify the type of error.

  2. +++ b/core/modules/views_ui/src/Tests/ViewEditTest.php
    @@ -239,4 +244,16 @@ public function testRelationRepresentativeNode() {
    +  /**
    +   * Override the error method so we can test for the expected exception.
    +   *
    +   * @todo Remove as part of https://www.drupal.org/node/2864613
    +   */
    +  protected function error($message = '', $group = 'Other', array $caller = NULL) {
    +    if ($group === 'User warning') {
    +      throw new \Exception($message);
    +    }
    +    return parent::error($message, $group, $caller);
    +  }
    

    It is necessary only for simpletest, and will be deleted with PHPUnit.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Nice.

Should we have a follow up to deal with the other debug statements in Views? IMO debug statements should not live in core code and as @dawehner pointed out in #11, this is probably a leftover from D6/PHP4 times, so it could do with an update to more current best practice.

vaplas’s picture

Thank you, @Lendude. Done.

Status: Reviewed & tested by the community » Needs work

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

Lendude’s picture

Part of this patch got into #2897009: MediaInterface is missing setName() and getName() so that is why this no longer applies.

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

#2897009: MediaInterface is missing setName() and getName() got fixed by @webchick, this applies again, so back to RTBC

  • catch committed 0fa72bd on 8.5.x
    Issue #2853359 by vaplas, Lendude, tinny: Runtime debug statement in...

  • catch committed 1a37350 on 8.4.x
    Issue #2853359 by vaplas, Lendude, tinny: Runtime debug statement in...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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