Respect tour status (disabled | enabled)

This is bad behavior on my part. Moving to RTBC as this is blocking a feature or two for tour UI. And even though tour UI is not part of core there is talk that tour is going to be slated for removal. Would be nice to get this feature in before that freeze happens.

Yes, we do not self RTBC in the Drupal core issue queue. I am setting this back to needs review per Reviewed & tested by the community ["RTBC"] → .

If this is block a contrib project, please add the correct tag.

For those interested, the proposal for discussing the removal of tour is at 🌱 [Policy] Remove tour module from core Fixed .

1. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,33 @@ public function testTourFunctionality() {
   +    // Navigate and verify the tour_test_1 tip is found with appropriate classes.
   

   nit: >80

2. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,33 @@ public function testTourFunctionality() {
   +    $this->assertTourTips([], TRUE);
   

   we can use a named argument here

3. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -45,7 +47,10 @@ public function assertTourTips($tips = []) {
   +      $this->assertTrue(empty($tips));
   

   can return here and avoid the elseif

Couple of minor things

Attempting to fix the issues mentioned in #33

#34 didn't address 2 or 3.

For 2 it was testing an empty but now it's passing in values.
For 3 Adding a return that way doesn't do anything as it's the end of the if statement.

Think (could be wrong) this was more what @larowlan was saying.

The last submitted patch, 34: 3004897-34.patch, failed testing. View results →

1. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,34 @@ public function testTourFunctionality() {
   +    // Navigate and verify the tour_test_1 tip is found with
   

   should this be 'is not found'?

2. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -45,7 +47,10 @@ public function assertTourTips($tips = []) {
   +    elseif (empty($tips)) {
   

   this can stay as an else, as an if, we returned in the above if path

Looks like #35.2 isn't handled yet, see https://php.watch/versions/8.0/named-parameters#named-param-optional-params for an example, I don't think we need to pass the empty array for the first argument as that's the default

Addressed points #37 and #35.2

1. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,34 @@ public function testTourFunctionality() {
   +
   

   This line can be removed (can be fixed on commit)

2. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,34 @@ public function testTourFunctionality() {
   +    $this->assertTourTips();
   

   Should we be passing the $tour here - to assert the one we expect to see is present?

3. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,34 @@ public function testTourFunctionality() {
   +    $this->assertTourTips(expectEmpty: TRUE);
   

   👌 noice

4. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -45,8 +47,8 @@ public function assertTourTips($tips = []) {
   -    if (empty($tips)) {
   -      $this->fail('Could not find tour tips on the current page.');
   +    if ($expectEmpty) {
   +      return;
   

   I think we want

   if (empty($tips) && $expectEmpty) {
     // No tips found as expected.
     return;
   }
   $tip_count = count($tips);
   if ($tip_count > 0 && $expectEmpty) {
     $this->fail(sprintf('No tips were expected but %d were found', $tip_count));
   }
   $this->assertGreaterThan(0, $tip_count);
   
   

   So we cover the three cases:

   * Expecting tips, but none
   * Not expecting tips, but found some
   * Not expecting tips, none found as expected

#39
1. Fixed
2. We can't as the $tour isn't an array
3. Thanks!
4. Updated

Test results: PASS

Notes:
- Tested the latest patch on Drupal 10.1.x using DrupalPod to setup up the env
- Tested with the umami_demo profile
- I had to uninstall and reinstall the Tour module for a tour itself to be disabled

Outcome:
1. Spun up the test environment with DrupalPod and logged in as admin
2. Verified that the Tour module was enabled in the testing site
3. Visually confirmed that tours were enabled and working by going to Languages in configuration (see Screenshot 1)
4. Changed status of tour to "false" in a yml file under language module and cleared cache
5. Checked the language tour in testing site but the tour was still visible
6. Uninstalled and then reinstalled the Tour module in the DrupalPod cli
7. Confirmed that only the language tour was disabled in the testing site (see Screenshot 2)
8. Re-enabled the language tour by setting status to “true” but it was only visible again in the testing site after I reinstalled the tour module

If you've tested and reviewed the ticket and think it's good to go feel free to change status to RTBC.

Taking #41 as a review.

This has been a block for tour_ui ability to setup tour statuses for a long time.

Committed and pushed to 10.1.x
I think this is borderline feature, so splitting the difference and marking as a task.

Didn't backport because we're in RC phase and I don't think this meets the requirements for commit during RC

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