Expand Tour with methods

@chrisjlee did you have any questions or intermediate work to post?

Yes. So would i usually (or most of the time) be adding two methods ->settings() and ->getSettings()

Here is my first pass.

4: tour_methods-2030661-4.patch queued for re-testing.

I did a review and everything looks ok.
I requested a re-test to make sure that everything is still good. If so, then I shall give this issue a rtbc.

4: tour_methods-2030661-4.patch queued for re-testing.

1. +++ b/core/modules/tour/lib/Drupal/tour/TourInterface.php
   @@ -41,4 +52,53 @@ public function getTip($id);
   +   * @param array $tips
   +   *   The tips for this hour.
   ...
   +   * @return array
   +   *   Return an array with collection of tips.
   ...
   +   * @param array $tips_bag
   +   *   The collection of tips for this hour.
   

   Given that tips are objects we can use a syntax like \Drupal\tour\TipPluginInterface[] instead of "array", so tools can leverage this information.

2. +++ b/core/modules/tour/lib/Drupal/tour/TourInterface.php
   @@ -41,4 +52,53 @@ public function getTip($id);
   +   * @return \Drupal\tour\Entity\Tour
   

   I guess we should use the TourInterface instead here.

3. +++ b/core/modules/tour/lib/Drupal/tour/TourInterface.php
   @@ -41,4 +52,53 @@ public function getTip($id);
   +   *   The Tour object.
   ...
   +   *   The Tour object.
   

   Afaik this lines should be proper sentences as well.

4. +++ b/core/modules/tour/lib/Drupal/tour/TourInterface.php
   @@ -41,4 +52,53 @@ public function getTip($id);
   +   *   Return module name
   

   s/Return/Returns the

re-roll plus creates new tipbag in setTips method, else I don't think it would work.

+++ b/core/modules/tour/lib/Drupal/tour/TourInterface.php
@@ -28,8 +28,8 @@ public function getPaths();
+   * @return self

@@ -58,46 +58,46 @@ public function getTips();
+   * @return self
...
+   * @return self
...
+   * @return self

Note: it is @return $this now.

Fixes docblock.

+++ b/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
@@ -90,7 +90,7 @@ public function __construct(array $values, $entity_type) {
 
   /**
-   * {@inheritdoc}
+   *
    */

That's weird? :)

Fixes #16

and interdiff

Looks good.

  /**
   * The module which this tour is assigned to.
   *
   * @var string
   */
  public $module;

Can this be protected now?

+++ b/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
@@ -123,6 +123,45 @@ public function getTips() {
+  public function setTips($tips) {
...
+  public function getTipsBag() {
...
+  public function setTipsBag($tips_bag) {
...
+  public function setModule($module) {

Why are we not adding any usages of these functions?

Changing status accordingly.

Hi,
here is the patch following #20 recommandations.

Thanks

Reroll

Value array ( 0 => 'tour_test', ) is equal to value array ( 0 => 'system', 1 => 'tour_test', ).

The test probably shouldn't expect 'system' anymore? I don't know why that changed though.

Removing 'system' from array fixes failing test.

-    $this->assertEqual($dependencies['module'], array('system', 'tour_test'));
+    $this->assertEqual($dependencies['module'], array('tour_test'));

Thank you, changed to {@inheritdoc}.

Thanks

1. +++ b/core/modules/tour/src/Entity/Tour.php
   @@ -41,7 +41,7 @@ class Tour extends ConfigEntityBase implements TourInterface {
   -  public $module;
   +  protected $module;
   

   Can we protect all the tour properties? Thanks.

2. +++ b/core/modules/tour/src/Entity/Tour.php
   @@ -123,6 +123,60 @@ public function getTips() {
   +  public function setTips($tips) {
   +    $this->set('tips', $tips);
   +    $this->tipsBag = new TipsBag(\Drupal::service('plugin.manager.tour.tip'), $this->tips);
   +    return $this;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getTipsBag() {
   +    return $this->tipsBag;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setTipsBag($tips_bag) {
   +    $this->set('tipsBag', $tips_bag);
   +    return $this;
   +  }
   ...
   +  /**
   

   These have no external usages are therefore unnecessary.

3. +++ b/core/modules/tour/src/Entity/Tour.php
   @@ -123,6 +123,60 @@ public function getTips() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setModule($module) {
   +    $this->set('module', $module);
   +    return $this;
   +  }
   

   This is only used in a test so kind of fake

4. +++ b/core/modules/tour/src/Entity/Tour.php
   @@ -123,6 +123,60 @@ public function getTips() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getExportProperties() {
   +    $properties = parent::getExportProperties();
   +    $names = array(
   +      'routes',
   +      'tips',
   +    );
   +    foreach ($names as $name) {
   +      $properties[$name] = $this->get($name);
   +    }
   +    return $properties;
   +  }
   

   This is totally unused code now.

Changed all properties to protected, removed unnecessary methods.

@lokeoke's patch applies cleanly.

In the code there are calls to the drupal_clean_css_identifier() function which has been marked as deprecated according to the docs. Shall we update the function call here or a mass update of the function call will be done later?

yeah let's fix those whilst we're touching those bits.

Ok, @larowlan. Replacing drupal_clean_css_identifier() calls in @lokeoke's patch.

+++ b/core/modules/tour/src/TourInterface.php
@@ -55,6 +55,14 @@ public function getTip($id);
+   *   The module to which this tour belongs to.

too many tos? The module to which this tour belongs - or - the module this tour belongs to?
Other than that, looks RTBC

New patch with @larowlan suggestion in #37.

Thanks @dinarcon

1. +++ b/core/modules/tour/src/TourViewBuilder.php
   @@ -28,9 +29,9 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   +              'tip-type-' . Html::cleanCssIdentifier($tip->get('plugin')),
   

   This could be just ->getPluginId().

2. +++ b/core/modules/tour/src/TourViewBuilder.php
   @@ -28,9 +29,9 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   +              'tip-' . Html::cleanCssIdentifier($tip->get('id')),
   

   I think this should be cleaned up still; TipPluginInterface should have a getId() (or id()) method, just like it already has getLabel(). But I guess that's out of scope… is there an issue for that yet, @larowlan?

Back to NR (not NW) so larowlan can determine whether this feedback should be addressed here or not.

Yep you're right, let's fix it here - thanks!

Patch with @Wim Leers notes in #40.

The method doesn't exist yet; It needs to be created.

Added methods and some cleanup in docs.

Looks good! :)

+++ b/core/modules/tour/src/TipPluginBase.php
@@ -42,28 +42,42 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getPluginId() {
+    return $this->get('plugin');
+  }

+++ b/core/modules/tour/src/TipPluginInterface.php
@@ -18,6 +18,22 @@
+  /**
+   * Returns plugin id of the tip.
+   *
+   * @return string
+   *   The plugin id of the tip.
+   */
+  public function getPluginId();

Do we actually need this? Is not the method on PluginBase good enough?

Also can we get a followup to remove the confusing unused properties from TipPluginBase, TipPluginText, and TipPluginImage which all have these properties that afaics are never used since ->get() gets them from the configuration property.

The patch probably needs a reroll.

+++ b/core/modules/tour/src/TipPluginBase.php
@@ -42,28 +42,42 @@
+  public function id() {
+    return $this->get('id');
+  }

+++ b/core/modules/tour/src/TipPluginInterface.php
@@ -18,6 +18,22 @@
   /**
+   * Returns id of the tip.
+   *
+   * @return string
+   *   The id of the tip.
+   */
+  public function id();

Why is this function necessary.

I'm not sure. Because it seems like the id of the tip, not the id of the plugin.

The problem is that the class TipPluginBase does not have a variable "id". As far as I can see.

@lokapujya: If you update the patch. I shall do the review.

Right, I assume that the id comes from the tours .yml file.

We probably should do the change in 48.

trying out #48.

I think I was supposed to remove it from TipPluginBase too. Will try that later.

TipPluginBase. *Ignore this patch*

Ignore the last patch, wrong change.

@lokapujya: I think that something is wrong with your patch file, because it is not being tested.

kick off the test.

All the class variables for Tour and TipPluginBase are protected.
The comments and the doc block are in order.
For the Tour class is the function getModule() added.
For the TipPluginBase is the function id() added.
There are no tests added for the two functions.
The patch is given all green from the test-server.
In the TourViewBuilder class there is a bit of code cleaning with the replacement of drupal_clean_css_identifier.
The patch get a RTBC from me.

The meta issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties has the beta evaluation form completed. Committed 737f04d and pushed to 8.0.x. Thanks!