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 ?

CommentFileSizeAuthor
#16 3532360-nr-bot_yo5j6nwy.txt1.58 KBneeds-review-queue-bot

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:

About issue forks

Comments

aaronbauman created an issue. See original summary.

aaronbauman opened merge request !12485

aaronbauman opened merge request !12486

aaronbauman’s picture

aaronbauman
he/him
commented
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.

ironnuts’s picture

ironnuts commented

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

aaronbauman’s picture

aaronbauman
he/him
commented

Oh, thanks for the heads up - TIL!

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

ironnuts’s picture

ironnuts commented

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.

ironnuts’s picture

ironnuts commented

@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

quietone commented
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

smustgrave commented
Status: Needs review » Needs work

MR needs to be updated for 11.x please.

aaronbauman opened merge request !12798

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

aaronbauman’s picture

aaronbauman
he/him
commented
Status: Needs work » Needs review

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3532360-nr-bot_yo5j6nwy.txt1.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

aaronbauman
he/him
commented
Status: Needs work » Needs review

smustgrave closed merge request !12485

smustgrave closed merge request !12486

smustgrave’s picture

smustgrave commented
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

aaronbauman
he/him
commented
Status: Needs work » Needs review

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

Thanks for the feedback.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks feedback appears to be addressed

catch’s picture

catch
he/him
Primary language English
commented
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

aaronbauman
he/him
commented
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

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

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

catch’s picture

catch
he/him
Primary language English
commented
Title: Check return value from getCurrentRequest() before calling setRequest() » Check return value from getCurrentRequest() before calling setRequest() in ViewExecutableFactory

catch closed merge request !12798

  • 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

catch
he/him
Primary language English
commented
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.