Add context from Layouts to be passed into to Views blocks.

smaiorana commented 1 year ago

Views blocks currently don't allow context to be passed in from a Layout. The only way to accomplish this now is to extract context from the URL based on position. This is less than ideal because it forces the block to be locked to a specific path structure that is separate from the Layout configuration.

Drupal 7 had the option to choose a data source "From context" in a View block. This code exists in Backdrop but has been commented out with a TODO. Perhaps this should be revisited to add context handling for Views blocks.

Update (April 3, 2023): The latest pull request for this issue adds context support to views blocks associated with layouts. There has been many revisions made to this pull request to address concerns from this wonderful Backdrop community. Some of these revisions have addressed improvements, efficiency changes, updated coding standards, etc. As of today, this PR has passed all the validation checks except one that seems to be some kind of external issue with that particular check.

Update (April 11, 2023): The latest pull request for this issue includes automated testing and is awaiting review. All validation tests have successfully passed.

Advocate for this feature: @rbargerhuff

TRANSLATION UPDATE:

Changed strings:

old (d7 views): Which type of context to use.
new: Select which type of context to use that best matches the contextual filter.

Removed strings:

Context is optional
This context need not be present for the block to function. If you plan to use this, ensure that the contextual filter handler can handle empty values gracefully.

klonos commented 1 year ago

Done @rbargerhuff 👍🏼 ...thanks for keep pushing this forward 🙏🏼

rbargerhuff commented 1 year ago

Just committed an update to our PR. This PR now includes automated testing.

Could someone please approve the PR so the validation tests would run? Thank you very much!!

rbargerhuff commented 1 year ago

Okay thank you @laryn for approiving the PR. I just submitted another update. Could you please approve the PR once more. Thank you very much!!

@bugfolder The PHPCS validation is failing on special comments. In our case it was the following:

//<editor-fold desc="Navigate to the configure page of our view and add fields to sort the page by.">
//</editor-fold>

It should ignore these lines because otherwise it will fail because there is no space after the //

Cheers!!

rbargerhuff commented 1 year ago

@laryn Could you re-run that test. They failed for reasons unbeknownst to us. We think the tests had some kind of internal failure.

@bugfolder Is it true that we are validating some kind of spell check?

Run streetsidesoftware/cspell-action@v2
cspell-action
Pull Request
Warning: Unknown word (collab)
core/modules/views/tests/views_layout.test:4:31 Unknown word (collab)
Files checked: 5, Issues found: 1 in 1 files.
Error: 1 spelling issue found in 1 of the 5 files checked.
Done.

laryn commented 1 year ago

@rbargerhuff Done. (Also: FYI, I believe once your first PR is merged you won't need approval for tests to run anymore)

The spell check is a recent addition: https://github.com/backdrop/backdrop-issues/issues/5811

quicksketch commented 1 year ago

Thanks @rbargerhuff, I also added you to the docs and organization GitHub group, which might fix the permissions for running tests. I meant to do that earlier but it must not have worked correctly. Check the email associated with your GitHub account to verify the invitation.

rbargerhuff commented 1 year ago

Thanks @rbargerhuff, I also added you to the docs and organization GitHub group, which might fix the permissions for running tests. I meant to do that earlier but it must not have worked correctly. Check the email associated with your GitHub account to verify the invitation.

Thank you very much. I accepted the invite link. I appreciate it!!

rbargerhuff commented 1 year ago

I had to delete the PR and start-over. I do not understand how to continue making code changes to my PR once there has been new commits made to Backdrop 1.x branch.

Could anyone offer assistance?

@herbdool Since the PR was deleted, I am putting this here in order to save the discussion:

<editor-fold> along with region comments allow the coder to define a section of text that is collapsible. Almost all IDEs and Text-Editors that support programming have built-in support for this text .

I feel we should support these seeing how there is an effort to improve the readability of code.

I would to openly discuss this with the community and hear their thoughts on this.

avpaderno commented 1 year ago

I had to delete the PR and start-over. I do not understand how to continue making code changes to my PR once there has been new commits made to Backdrop 1.x branch.

For that, it should be sufficient to synchronize the PR branch with Backdrop branch.

avpaderno commented 1 year ago

If there are not conflicting changes, that should not be a problem.

rbargerhuff commented 1 year ago

If there are not conflicting changes, that should not be a problem.

But then I cannot move my latest changes to the top of the commit logs in order to create a clean PR like it's suggested in @quicksketch 's documentation: https://docs.backdropcms.org/documentation/contribute-to-backdrop-core

rbargerhuff commented 1 year ago

Hello everyone, I require some assistance with a check that seems to be failing for an unknown reason:

---- Views: Layouts and Views context (ViewsLayoutsTest) ----

Status    Group      Filename          Line    Function                            
------------------------------------------------------------------------------------------------------------------------
Fail      Other      views_layout.test 16      setup()                                                                 
    Enabled modules: views, views_ui, layout, entity_ui, block, user, contextual
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Exception Warning    views_plugin_disp 287     options_form()                                                          
    Undefined array key "hidden"
Error: Process completed with exit code 1.

Can anyone offer any assistance with this?

argiepiano commented 1 year ago

Why are you trying to enable entity_ui? That's a contrib module that should not be used for testing core functionality.

That's failing because the github action used for doing functional tests doesn't "checkout" entity_ui or any other contrib module (only Backdrop core). This means that the module entity_ui can't be enabled since it's not available. That probably is creating the other issues down the line.

rbargerhuff commented 1 year ago

Why are you trying to enable entity_ui? That's a contrib module that should not be used for testing core functionality.

That's failing because the github action used for doing functional tests doesn't "checkout" entity_ui or any other contrib module (only Backdrop core). This means that the module entity_ui can't be enabled since it's not available. That probably is creating the other issues down the line.

Thank you for this input! I am going to consult with my colleague who helped write the test and see why we are enabling entity_ui

rbargerhuff commented 1 year ago

@klonos or anyone with "super powers" Could you please update the first comment of this issue with the following:

Update (April 11, 2023): The latest pull request for this issue includes automated testing and is awaiting review. All validation tests have successfully passed.

rbargerhuff commented 1 year ago

The latest PR is ready for review. Finally all the validation tests have successfully passed ':-)

klonos commented 1 year ago

Done @rbargerhuff 👍🏼

As for your troubles with syncing your PR branch with the latest 1.x, I have a script on my local, that roughly does the following:

It starts by cloning the official backdrop repo (NOT from my klonos/backdrop fork - always the "origin" backdrop/backdrop repo - keep reading to see why): git clone https://github.com/backdrop/backdrop docroot && cd docroot
Adds my fork of backdrop as a remote: git remote add klonos https://github.com/klonos/backdrop
(force-)Pushes latest 1.x to my fork (so that my fork has an up-to-date 1.x branch, in sync with the 1.x branch of the "origin" official repo - I never touch the 1.x branch in my repo): git push -u klonos 1.x:1.x --force
It then fetches the PR branch on my local origin: git fetch origin pull/[NUMBER_OF_PR_ON_GITHUB]/head:[NAME_OF_PR_BRANCH]
Then, it checks out the PR branch: git checkout [NAME_OF_PR_BRANCH]
Then, it stashes any local changes I may have: git stash
Then, it merges origin 1.x into the PR branch: git merge origin/1.x -m "Merge 'origin/1.x' into klonos/[NAME_OF_PR_BRANCH]"
It finally re-applies any stashed changes: git stash apply

The key difference is that when I want to push my changes, the default origin is not where I want to push - I always need to "manually" push to my fork/branch with the following: git push -u klonos [NAME_OF_PR_BRANCH]

...if the name of the local branch for some reason is different to the remote, then the above command should be: git push -u klonos [LOCAL_BRANCH]:[REMOTE_PR_BRANCH]

(of course you will need to use the name of your own GitHub repo or organization instead of klonos in the above commands 😉 )

The reason why I never touch my 1.x should be obvious now: when I want to start working on a fresh issue, the script has made sure that my forked 1.x has what's in origin 1.x. Re-running the script always refreshes my fork's 1.x, so when I checkout a new branch to work of, that has the latest 1.x.

YMMV of course, but I have been using this script for years now, so I hope that the above helps.

quicksketch commented 1 year ago

Thanks @rbargerhuff! I'll be reviewing this PR in the next couple of days. We're so close on this one we'll definitely want to get it into the 1.25.0 release. The feature freeze is Monday, so we'll want to get this merged before then.

quicksketch commented 1 year ago

I gave the PR at https://github.com/backdrop/backdrop/pull/4398 a full review and test in the sandbox, it works great! Great decisions and implementation all-around. Amazing job @rbargerhuff!

I found only very minor code-style issues in the PR. @rbargerhuff, if you get the opportunity to incorporate the changes then I think this is good to go. If you don't have the time to get to it, I can add the code style fixes to the PR myself tomorrow.

quicksketch commented 1 year ago

Looks like all requested changes have been made. I think this is ready to go!

rbargerhuff commented 1 year ago

Hi @quicksketch , thank you for the wonderful compliments! I will be sure to pass them along to @smaiorana since he and I completed the work together.

I made the changes you suggested but I do not know how to limit the commit for this PR to a single commit in the repo's log. I just cannot figure out how to do this without corrupting the repo.

quicksketch commented 1 year ago

@rbargerhuff No problem with the commit log. When I merge I use GitHub's "Squash and Merge" to make it into a single commit.

I'll let the automated tests run one more time.

rbargerhuff commented 1 year ago

@quicksketch Here are my commits that I guess should be squashed.

commit cc555ccc0beffe492854e9588a85f1d06b6a83f2 (HEAD -> 5995/views-layout-context-integration, origin/5995/views-layout-context-integration)
Author: Rick Bargerhuff <13067709+rbargerhuff@users.noreply.github.com>
Date:   Mon May 1 15:35:01 2023 -0400

    Issue #5995 continued: Includes the latest suggestions offered by the Backdrop community.

commit b4a2a9c7cb22d12ca39352b8a74cd7bc7e906317
Merge: 36e070d30 01595f295
Author: Rick Bargerhuff <13067709+rbargerhuff@users.noreply.github.com>
Date:   Mon May 1 15:31:05 2023 -0400

    Merge branch 'backdrop:1.x' into 5995/views-layout-context-integration

commit 36e070d30e3fe9d9fa84e666ae9fb9cf78d213b7
Author: Rick Bargerhuff <13067709+rbargerhuff@users.noreply.github.com>
Date:   Mon Apr 10 11:45:30 2023 -0400

    Issue #5995: Adds context support to Views blocks associated with Layouts.

quicksketch commented 1 year ago

https://github.com/backdrop/backdrop/pull/4398 has been merged into 1.x for 1.25.0. Great work once again @rbargerhuff! There's a long list of credits on this one!

https://github.com/backdrop/backdrop/commit/ab54b40bd4d6e7fa2002b49f7049110fccb9d4f3 by @rbargerhuff, @smaiorana, @docwilmot, @klonos, @quicksketch, @bugfolder, @herbdool, @laryn, @BWPanda, @kiamlaluno, and @argiepiano.

Sending a huge thank you to everyone that helped. This was an amazing collaboration. Thank you to every contributor!

backdrop / backdrop-issues

Add context from Layouts to be passed into to Views blocks. #5995

Advocate for this feature: @rbargerhuff