Problem/Motivation

The method \Drupal\views\ViewExecutableFactory::get raises an error when a session is not available - for example when invoked via drush.

\Symfony\Component\HttpFoundation\RequestStack::getCurrentRequest can return NULL, but the return value is not checked before passing it as an argument to \Drupal\views\ViewExecutable::setRequest.

Proposed resolution

  • Check return value from getCurrentRequest before calling setRequest

Remaining tasks

  • Test coverage to demonstrate issue
  • Update method

User interface changes

none

Introduced terminology

none

API changes

none

Data model changes

none

Release notes snippet

N/A ?

Issue fork drupal-3532360

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

aaronbauman created an issue. See original summary.

aaronbauman’s picture

Status: Active » Needs review

3532360-TEST-ONLY MR !12485 - test only to demonstrate bug.

3532360-check-for-session MR !12486 - includes test and fix, ready for review

oily changed the visibility of the branch 3532360-TEST-ONLY to hidden.

oily changed the visibility of the branch 3532360-TEST-ONLY to hidden.

oily’s picture

@aaronbauman A separate 'test-only' branch is not necessary. There is a 'test-only' manually activated test in the pipeline.

aaronbauman’s picture

Oh, thanks for the heads up - TIL!

Is that a configuration for core only, or for all projects?

oily’s picture

There is a gitlab template that can be used in projects. Can use it at default settings or vary them. I think most projects will have test-only test in pipeline unless the maintainer had a good reason to exclude it.

oily’s picture

@aaronbauman you also probably need to create a new 11.x branch and merge into that. Core maintainers then decide which branches to port to.

quietone’s picture

Version: 11.2.x-dev » 11.x-dev

In Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Also mentioned on the version section of the list of issue fields documentation.

smustgrave’s picture

Status: Needs review » Needs work

MR needs to be updated for 11.x please.

aaronbauman changed the visibility of the branch 3532360-check-for-session to hidden.

aaronbauman’s picture

Status: Needs work » Needs review

Opened a clean Merge request !12486 against 11.x for review

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.58 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

aaronbauman’s picture

Status: Needs work » Needs review

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Bug Smash Initiative

Ran the test-only run here https://git.drupalcode.org/issue/drupal-3532360/-/jobs/6995154 to show the coverage

Left a comment on the MR.

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

aaronbauman’s picture

Status: Needs work » Needs review

Merged 11.x again, and updated per your comment.

Thanks for the feedback.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks feedback appears to be addressed

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The MR doesn't match the issue title or summary at all.

aaronbauman’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Sorry, big copy-pasta error on my part.
Updated IS.

aaronbauman’s picture

Title: Check for session before calling getSession(), or catch SessionNotFoundException » Check return value from getCurrentRequest() before calling setRequest()
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

That was my mistake for not catching before but title looks much better

catch’s picture

Title: Check return value from getCurrentRequest() before calling setRequest() » Check return value from getCurrentRequest() before calling setRequest() in ViewExecutableFactory

  • catch committed 63485205 on 11.x
    fix: #3532360 Check return value from getCurrentRequest() before calling...

  • catch committed b13c0284 on main
    fix: #3532360 Check return value from getCurrentRequest() before calling...
catch’s picture

Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, 11.x and 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed b82be539 on 11.3.x
    fix: #3532360 Check return value from getCurrentRequest() before calling...

Status: Fixed » Closed (fixed)

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