Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Suresh Prabhu Parkala’s picture

Status: Active » Needs review
FileSize
6.71 KB

Please review.

Rajab Natshah’s picture

Status: Needs review » Needs work


Drupal coding standard and Drupal Practice check should pass

PHPCS, PHPCBF for both --standard=Drupal and --standard=DrupalPractice

Check Tour Builder Drupal coding standard

phpcbf --standard=Drupal --extensions=php,module,inc,install,test,theme,scss,css,info,txt,md,yml  /var/www/html/modules/tour_builder/

phpcs --standard=Drupal --extensions=php,module,inc,install,test,theme,scss,css,info,txt,md,yml /var/www/html/modules/tour_builder/

Check Tour Builder Drupal Practice

phpcbf --standard=DrupalPractice --extensions=php,module,inc,install,test,theme,scss,css,info,txt,md,yml  /var/www/html/modules/tour_builder/

phpcs --standard=DrupalPractice --extensions=php,module,inc,install,test,theme,scss,css,info,txt,md,yml /var/www/html/modules/tour_builder/


Rajab Natshah’s picture

FileSize
10.22 KB
Rajab Natshah’s picture

Rajab Natshah’s picture

Status: Needs work » Needs review
Rajab Natshah’s picture

Priority: Normal » Critical

Please if you can spare time to commit this issue to the dev branch that will be nice.
A release after that will have a support for Drupal 9 build.
as this issue is a blocker issue for
#3122672: Start a 9.0.x branch for Varbase and Varbase Project to integrate with Drupal 9

Real physical testing with Drupal 9 is important as Drupal 9.0.4 went out

Thanks for having time to work on this nice and very useful module.

Rajab Natshah’s picture

Status: Needs review » Needs work
[Wed Sep 02 17:45:35.158555 2020] [php7:notice] [pid 13988] [client ::1:36844] Uncaught PHP Exception Symfony\\Component\\DependencyInjection\\Exception\\ServiceNotFoundException: "You have requested a non-existent service "entity.manager"." at /var/www/html/dev/drupal9c1tour_builder/web/core/lib/Drupal/Component/DependencyInjection/Container.php line 151, referer: http://localhost/dev/drupal9c1tour_builder/web/admin/config
Rajab Natshah’s picture

FileSize
10.6 KB
Rajab Natshah’s picture

Status: Needs work » Needs review
Rajab Natshah’s picture

clemens.tolboom’s picture

Assigned: Unassigned » clemens.tolboom

@RajabNatshah thanks for spending time on this.

You're the first who adds the commands ran which is how it should be :-)

I think I saw removed comments which should not be removed as I want to restore the patch workflow one day.

What non automated changes do I have to make as you changed code between #4 and #9 ... an innerdiff would be handy.

Rajab Natshah’s picture

Thanks a lot Clemens for having time to follow on this issue.

Noted;

About the Batch workflow.
I had a look at the code for the Layout Builder Block Sanitizer
Learned a lot from them
The Batch class and services, and route

A new issue for that for sure.
I wanted to create the Batch issue but I had no extra time.

Thank you :)

Rajab Natshah’s picture

FileSize
11.12 KB

Changed the patch based on Clemens last review.
Thank you :)

Rajab Natshah’s picture

Rajab Natshah’s picture

Status: Needs review » Reviewed & tested by the community
Rajab Natshah’s picture

I hope that you do have time to commit this issue
Thank you for maintaining this module.
a Release after this issue will have the module work under Drupal 9

Rajab Natshah’s picture

My hope to commit this issue and a new tag release
I had to #3176580: [TEMP] Remove [Tour Builder] module from the composer on the 9.0.x branch
This module is very helpful to work on building tours in projects
I like to have it work under Drupal 9 websites
#3122672: Start a 9.0.x branch for Varbase and Varbase Project to integrate with Drupal 9

clemens.tolboom’s picture

I skipped the following changes

git diff
diff --git a/src/Form/TourBuilderExportForm.php b/src/Form/TourBuilderExportForm.php
index aafea0c..6457a57 100644
--- a/src/Form/TourBuilderExportForm.php
+++ b/src/Form/TourBuilderExportForm.php
@@ -60,8 +60,6 @@ class TourBuilderExportForm extends EntityForm {
   public function form(array $form, FormStateInterface $form_state) {
     $form = parent::form($form, $form_state);
 
-    $tour = $this->entity;
-

     $definition = $this->entityManager->getDefinition('tour');
     $name = $definition->getConfigPrefix() . '.' . $this->entity->getOriginalId();

That was in for a reason I cannot remember so need to test first

diff --git a/tour_builder.install b/tour_builder.install
index 6cb0ba3..bef013e 100644
--- a/tour_builder.install
+++ b/tour_builder.install
@@ -11,6 +11,4 @@
 function tour_builder_install() {
   // Set higher then tour_ui or tour module.
   module_set_weight('tour_builder', 1);
-  // TODO: this does not work
-  //Drupal::cache()->invalidateAll();
 }

This made installing tour_builder not updating the Action buttons on Tour UI page so is that still the case. Needs testing

diff --git a/tour_builder.module b/tour_builder.module
index 8558ee6..b1e35c7 100644
--- a/tour_builder.module
+++ b/tour_builder.module
@@ -5,8 +5,6 @@
  * Core functionality for Tour UI module.
  */
 
-use Drupal\Core\Entity\EntityInterface;
-
 /**
  * Implements hook_entity_type_alter().
  */

PHP Storm complained about this ... why?

clemens.tolboom’s picture

With composer require drupal/tour_builder:1.x-dev I was able to test manually.

Export gives an error on /admin/config/user-interface/tour/manage/views-ui/export

The website encountered an unexpected error. Please try again later.

Not sure that was already an issue but added #3185215: Tour export broken

clemens.tolboom’s picture

Status: Reviewed & tested by the community » Fixed
Related issues: +#3185215: Tour export broken

I call this one fixed now.

Thanks for your patience :-)

Status: Fixed » Closed (fixed)

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