Move methods from TourTipPluginInterface and deprecate this interface.

Please include interdiffs with your patches. fyi

Fyi you should include interdiffs for all your patches. No easy way to tell what's changed between your patches thus delaying this.
Also the patch names should be updated for the comment

Failures in the patch in #11 so moving to NW.

All the patches are same only the difference of the file path as patch was failed to apply earlier. I have included the interdiff in #10

Additionally the patches are failing in #11 but I ran the retest again and it successfully passed.

If you see the error you’ll get to know due to date mismatch test failed just because after 12 midnight day changes.

Reviewed patch #15 on drupal version -10.0.x-dev. The patch was successfully applied and removed TourTipPluginInterface file and Moved all methods from TourTipPluginInterface to TipPluginInterface. Need RTBC+1.

Good to move to RTBC.

This needs an additional person to RTBC it.

+++ /dev/null
@@ -1,58 +0,0 @@
-interface TourTipPluginInterface extends TipPluginInterface {

The ideal here would be deprecate this interface instead of remove it directly. You can do something like:

<?php

namespace Drupal\tour;

@trigger_error('The ' . __NAMESPACE__ . '\TourTipPluginInterface is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Implement ' . __NAMESPACE__ . '\TipPluginInterface instead. See {CHANGE RECORD}.', E_USER_DEPRECATED);

/**
 * Defines an interface for tour items.
 *
 * @see \Drupal\tour\Annotation\Tip
 * @see \Drupal\tour\TipPluginBase
 * @see \Drupal\tour\TipPluginManager
 * @see plugin_api
 * 
 * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. The TourTipPluginInterface were moved
 * to TipPluginInterface.
 *
 * @see {CHANGE RECORD}
 */
interface TourTipPluginInterface extends TipPluginInterface {}

It also needs tests for the deprecation part, for that you can do something like:

<?php

namespace Drupal\Tests\tour\Kernel;

use Drupal\tour\TourTipPluginInterface;
use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;

/**
 * @coversDefaultClass \Drupal\tour\TourTipPluginInterface
 * @group tour
 * @group legacy
 */
class TourTipLegacyTest extends TestCase {

  use ExpectDeprecationTrait;

  public function testPluginHelperDeprecation(): void {
    $this->expectDeprecation('The Drupal\tour\TourTipPluginInterface is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Implement Drupal\tour\TipPluginInterface instead. See {CHANGE RECORD}.');
    $plugin = $this->createMock(TourTipPluginInterface::class);
    $this->assertInstanceOf(TourTipPluginInterface::class, $plugin);
  }

}

That's it, I'm moving this back to NW, for the deprecation part, tests and the change record.

Interdiff is same as mentioned in #22

Tests and Change record

@smustgrave what changes exactly can you please elaborate?

Removing tests tag. Would also help to put a comment of what you are doing between patches in addition to an interdiff, to quickly see you added tests.

Still needs a change record, which would then need to be added to the patch.

Did not do a code review.

@smustgrave Still needs a change record, which would then need to be added to the patch. -> Please help me with this I have added the tests and refactor the whole interface also, if there's anything else missing please let me know.

Hey @ankitsingh0188, you can check more about CR in: https://www.drupal.org/community/contributor-guide/task/write-a-change-r... → , and please, do not remove the tag without creating it.

Anyway, I've updated the patch with the new CR, removed the unnecessary implements. Moving back to NR

Hi @murilohp

it's better if you add the interdiff between #24 & #29.

it's better if you add the interdiff between #24 & #29 because in interdiff we generally include the comment number and as per your interdiff it's between #6 & #29

Thanks for the review! The interdiff is actually from #24, but since the name of patch was 3276336-6.patch, I've named the interdiff accordingly.

Here's a new interdiff between #24 and #29 renamed.

1. +++ b/core/modules/tour/src/TipPluginBase.php
   @@ -118,4 +118,18 @@ public function getSelector(): ?string {
   +  /**
   +   * Returns the body content of the tooltip.
   +   *
   +   * This typically maps to the Shepherd Step options `text` property.
   +   *
   +   * @return array
   +   *   A render array.
   +   *
   +   * @see https://shepherdjs.dev/docs/Step.html
   +   */
   

   Can @inheritDoc

2. +++ b/core/modules/tour/src/TourTipPluginInterface.php
   @@ -10,49 +12,9 @@
   + * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Implements
   

   I think this should be Implement not Implements

NW for minor fixes, otherwise RTBC.

I have updated the patch as per #34 📌 Move methods from TourTipPluginInterface and deprecate this interface. Fixed .

The last submitted patch, 36: 3276336-35.patch, failed testing. View results →

I was reviewing #38 and #34 was addreses, the patch looks and it's applying, so for me it's RTBC now.

I was looking at the two plugins that remove the interface and wondering whether they need to continue implementing it in case someone was using it as a type hint, but no one should be type hinting plugin implementations like that especially on an interface that not all plugins of that type implement anyway, so it's fine.

Committed/pushed to 10.1.x, thanks!

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

Published the CR