Drupal 9 Deprecated Code Report

Here is the patch, please review.

Against flag-8.x-4.x-dev (18 Oct 2018) there are still a couple of depreciated function and methods:

 ------ --------------------------------------------------- 
  Line   flag.install                                       
 ------ --------------------------------------------------- 
  65     Call to deprecated function drupal_set_message().  
 ------ --------------------------------------------------- 

 ------ ------------------------------------------------------------------- 
  Line   src/Form/FlagFormBase.php                                          
 ------ ------------------------------------------------------------------- 
  320    Call to deprecated method l() of class Drupal\Core\Form\FormBase.  
  324    Call to deprecated method l() of class Drupal\Core\Form\FormBase.  
 ------ ------------------------------------------------------------------- 

                                                                                
 [ERROR] Found 3 errors                                                         

Thank you @waverate

Patch at #4 works. Thank you @Sergiu Stici.

Removing the drupal_set_message() usages are the subject of the much older issue #2906147: Replace drupal_set_message . The removal should be handled in that issue, not here. Please remove that part of the patch, including the unneeded injection of the messenger service.

Also, assertEquals() does not have three arguments like assertEqual() did, and the order of the first two arguments is different between these two functions. That should be corrected.

Please review patch

Didn't address #6 ....
Also, you ADDED two new coding standards violations ...

Reverted 'drupal_set_message' change as mentioned in #6 and fixed introduced coding standards violations.

Thank you all for working on this ...

There is a delicate balance about how to accept good changes that are maybe out of scope, and how do we reduce duplicate work.

For example in the sister issue below about refining drupal_set_issue. Is asking should we accept a out of scope change which will cause conflicts in this issue?

https://www.drupal.org/project/flag/issues/2906147#comment-13323942

I think the answer is yes

I think given the smaller number of people submitting patches to the module ...we should error in away that encourages work and accepts patches that have consequences for other issues.

I will give a commitment to pounce on problems - reroll and resolve conflicts with a quick turnaround. An so please contact me if you see something I did not catch.

1. +++ b/src/ActionLink/ActionLinkTypePluginInterface.php
   @@ -5,12 +5,12 @@ namespace Drupal\flag\ActionLink;
     * Provides an interface for link type plugins.
     */
   -interface ActionLinkTypePluginInterface extends PluginFormInterface, ConfigurablePluginInterface {
   +interface ActionLinkTypePluginInterface extends PluginFormInterface, ConfigurableInterface {
   

   This is an 8.7 deprecation, so we should combine it with a dependency in flag.info.yml: - drupal:system (>=8.7)

2. +++ b/src/Form/FlagFormBase.php
   @@ -302,7 +316,7 @@ abstract class FlagFormBase extends EntityForm {
    
        $status = $flag->save();
   -    $url = $flag->urlInfo();
   +    $url = $flag->toUrl();
        if ($status == SAVED_UPDATED) {
   

   this was now also fixed in the other issue and can be removed.

3. +++ b/src/Tests/FlagTestBase.php
   @@ -154,7 +155,7 @@ abstract class FlagTestBase extends WebTestBase {
       */
      protected function assertContextualLinkPlaceHolder($id) {
   -    return $this->assertRaw('<div' . new Attribute(['data-contextual-id' => $id]) . '></div>', format_string('Contextual link placeholder with id @id exists.', ['@id' => $id]));
   +    return $this->assertRaw('<div' . new Attribute(['data-contextual-id' => $id]) . '></div>', new FormattableMarkup('Contextual link placeholder with id @id exists.', ['@id' => $id]));
      }
    
      /**
   @@ -169,7 +170,7 @@ abstract class FlagTestBase extends WebTestBase {
   
   @@ -169,7 +170,7 @@ abstract class FlagTestBase extends WebTestBase {
       *   The result of the assertion.
       */
      protected function assertNoContextualLinkPlaceholder($id) {
   -    return $this->assertNoRaw('<div' . new Attribute(['data-contextual-id' => $id]) . '></div>', format_string('Contextual link placeholder with id @id exists.', ['@id' => $id]));
   +    return $this->assertNoRaw('<div' . new Attribute(['data-contextual-id' => $id]) . '></div>', new FormattableMarkup('Contextual link placeholder with id @id exists.', ['@id' => $id]));
      }
   

   The thing with these arguments is that in phpunit/AssertLegacyTrait, assertNoRaw()/assertRaw() doesn't even have a second argument anymore. So I'd suggest to just remove that second argument completely. The HTML pretty much speaks for itself I think.

4. +++ b/tests/src/FunctionalJavascript/FlagContextualLinksTest.php
   @@ -190,7 +191,7 @@ class FlagContextualLinksTest extends WebDriverTestBase {
        $contextual_edit = $assert_session
          ->waitForElementVisible('css', 'div[data-contextual-id="' . $flag_contextual_links_id . '"] button');
   -    $this->assertNotNull($contextual_edit, format_string('Contextual link placeholder with id @id exists (flag).', ['@id' => $flag_contextual_links_id]));
   +    $this->assertNotNull($contextual_edit, new FormattableMarkup('Contextual link placeholder with id @id exists (flag).', ['@id' => $flag_contextual_links_id]));
    
   

   This one I'm not sure. This failing would likely be a bit less readable, but there are also examples in core without a message: \Drupal\Tests\file\FunctionalJavascript\FileFieldWidgetTest::testMultiValuedWidget.

   Fine to keep it like this for now.

Also, this is missing a few things that drupal-check doesn't see.

* entity.manager usage in \Drupal\flag\Tests\AdminUITest::doFlagChangeWeights
* isSubclassOf() in \Drupal\flag\Plugin\Flag\EntityFlagType::isFlaggableOwnable

And then also that we need to convert the web tests to phpunit, but lets do that in a separate issue.

Regarding #13

I think we should fix things in isolated semi-quick fix, quick to review side issues.

[converting tests may be a less quick to fix.]

Addressing #12
1. ✅Added dependency on drupal:system (>=8.7)
2. ✅Rerolled the patch.
3. ✅Removed second argument in each of these.
4. Left alone for now.

Adding a follow-up: #3090414: Fix coding standards

I applied the patch #16 and there are no errors showing up.

[OK] No errors

+++ b/flag.info.yml
@@ -3,6 +3,8 @@ description: Create customized flags that users can set on entities.
 type: module
 package: Flags
+dependencies:
+ - drupal:system (>=8.7)

Sorry for moving the goalpost, but I've started to instead replace the core definition with core_version_requirement: ^8.7.7 || ^9, then we're basically already ready for D9 already as far as the info.yml file is concerned.

Speaking of that, there is for example the new required $defaultTheme property that we can already add because it doesn't break on 8.7. Similar with the more strict assertTrue() in LinkTypeAjaxTest::testAjaxLink(), note that you can only see that when actually running the test with visible deprecation messages. And there is a separate issue for TrustedCallbackInterface already.

Also needs a reroll it seems.

All excellent feedback ...

the ^8.7.7 || ^9 comment accidentally solves a problem ... that has been animating me in another project. thank you.

First things first. I am about to reroll.

Reroll.

Regarding #20 ..This still needs work.

I am not confident about many of these changes, so I am going to talk through my decisions.

A)

the new required $defaultTheme property

so according to this change record

https://www.drupal.org/node/3085950

stark should be used in favour to classy...

B) Looking at the deprecation warnings from LinkTypeAjaxTest

I changed the 'computed' entity reference in the Flagging entity.

$fields['flagged_entity'] = BaseFieldDefinition::create('entity_reference')
->setLabel(t('Entity'))
->setDescription(t('The flagged entity.'))
- ->setComputed(TRUE);
+ ->setComputed(TRUE)
+ ->setClass(FlaggedEntityFieldItemList::class);

adding a new class FlaggedEntityFieldItemList

There should be only be one entity associated with the individual flagging.

I am really not sure about that .. and it may be additional tests -- but well I should wait for feedback before thinking about that.

1. +++ b/flag.info.yml
   @@ -3,6 +3,8 @@ description: Create customized flags that users can set on entities.
    package: Flags
   +dependencies:
   + - drupal:system (^8.7 || ^9)
    test_dependencies:
     - rules:rules
   

   this needs to be set as core_version_requirement, instead of core, not as a drupal_system dependency. See https://www.drupal.org/node/3070687.

2. +++ b/src/Plugin/Field/FlaggedEntityFieldItemList.php
   @@ -0,0 +1,22 @@
   +   * Returns the label as the field item.
   +   */
   +  protected function computeValue() {
   +    $this->list[0] = $this->createItem(0, $this->getEntity()->label());
   +  }
   

   This seems strange.

   $this->getEntity() returns the parent flagging entity, but then setting the label as a value of that doesn't make sense.

   You can see that this is usually set in \Drupal\flag\Entity\Flagging::onChange() and \Drupal\flag\Entity\Flagging::__construct(). Doing it in the field item list class should replace that logic, so you want to set the value of the entity_id field when it's accessed, not the label.

@bedir thank you for talking constructively about my screw ups ... it is always appreciated.

From #20

And there is a separate issue for TrustedCallbackInterface already.

#3090426: use TrustedCallbackInterface in FlagLinkBuilder::build

I started both issues... I was intending this issue to be a meta with lots of spin offs
but I don't want to undo all the good work put into this issue/patch. So I am going with the flow and folding
that patch into this one.

#24.1 Opps my mistake.

So we want to be compatible with 8.7.7 onwards ..
I think this is what is required. Note the last point in the CR is that we need a composer.json file.

#24.2

Doing it in the field item list class should replace that logic, so you want to set the value of the entity_id field when it's accessed,

Sorry If I need to talk this over some more...

To put the logic in the Flagging::onChange() in a human relatable form

Conditions
A) When changing the 'entity_id' field also update appropiately the 'flagged_entity' field.
B) Thow expections if this update/onchange operation is set more than once.

I did have a mini design review, on the basis that these fields are inherently linked
so maybe we should create a compound field ( ie follow and adapt this tutorial https://www.lullabot.com/articles/extending-a-field-type-in-drupal-8
) where instead of adding a 'quantity' associated with entity_reference we have logic to enforce the conditions.

But when I looked at it -- what we have already is a consise solution to the problem.
It is just a subltley that we apply the conditions the the level of a ContentEntityType [ Flagging ]
not at a level below where a developer fresh to the project may look first.

  public function onChange($name) {
    if ($name == 'entity_id' && $this->get('flagged_entity')->isEmpty()) {
      $this->flagged_entity->target_id = $this->entity_id->value;
    }
    if (in_array($name, ['flagged_entity', 'entity_id']) && $this->flagged_entity->target_id != $this->entity_id->value) {
      throw new \LogicException("A flagging can't be moved to another entity.");
    }
    parent::onChange($name);
  }

When it comes to testing the new class FlaggedEntityFieldItemList .. I want to say it is remarkable that this actually passed.

+  protected function computeValue() {
+    $this->list[0] = $this->createItem(0, $this->getEntity()->label());
+  }

I mean swapping the label() for something hardcoded to fail like the string 'socks'
is illuminating...

So much so that for this patch I wanted to see how many of our existing tests passed through FlaggedEntityFieldItemList.

So :-

a) I turned on xdebug run
b) ./vendor/bin/phpuinit -c core --coverage-html coverage module/contrib/flagged_entity
c) waited 4 hours .. and produced a static website showing out code coverage. ( single threaded so I didnt lock up my machine )

From a review of the code coverage report I see that testing of thrown expections is systematically missing from the flag module.
I looked first at out services FlagService and CountManager -- we would get 100% if it weren't for that fact.

I will post a code coverage report of the 8.x-4.x branch on github later today for anyone intrested.

[ Side note we also define create() methods inappropately in a couple of classes hampering our code coverage score.
So I spun off a #3096449: Remove FlagCountManager/FlagTypeBase :: create()
]

In short, regarding testing

A) I am happy that out existing tests will cover any future break of the new class before we get to RTBC.
B) I am thinking of adding thrown exception testing in a subsequent issue.

So now onto new changes -- sorry for the marathon.

I added more of the following to FunctionJavascript classes.

+ /**
+ * {@inheritdoc}
+ */
+ protected $defaultTheme = 'stark';
+

But again, minor complications in a few places to get the test to pass I had to resort to 'classy' over stark
looking at this CR

https://www.drupal.org/node/3083055

It talks about adding 'classy' lead to a short term fix.

This issue is turning into a mega patch.

We have another issue

#3038815: [Meta] Test Refactor

Which has a impact on those changes ... and so I think 'classy' here and deal with any issue down the line.

Lastly I was seeing a few warnings regarding assertTrue()/assertFalse() no longer being good enough
so assetNotNull()/assertNull() are required.

-    $this->assertTrue($this->flagService->getFlagging($this->flag, $this->node, $auth_user));
+    $this->assertNotNull($this->flagService->getFlagging($this->flag, $this->node, $auth_user));

Hi. I was working on deprecated function report and finished it. When I wanted to upload the patch I saw this issue, took a look into changes and were nearly identical to mine for that issue. So I decided to review this patch instead.

Found 2 CS issues for unused use statements.

Ran drupal-check,

  Line   src/FlagLinkBuilder.php
 ------ ------------------------------------------------------------------------------------------------------
         Class Drupal\flag\TrustedCallbackInterface was not found while trying to analyse it - autoloading is
         probably not configured properly.

which is caused by

// @codingStandardsIgnoreStart
// @todo remove this BC layer once support for Drupal 8.7 is sunsetted
if (interface_exists('\Drupal\Core\Security\TrustedCallbackInterface')) {
  interface TrustedCallbackInterface extends \Drupal\Core\Security\TrustedCallbackInterface{};
  }
  else {
    interface TrustedCallbackInterface{}
  }
// @codingStandardsIgnoreStop

However running drupal-check -d reports a lot of errors, most of them are type hints and false positives. Should we take a look into it in this issue or another?

Yes, no coding standards checks here, this is complicated enough as it is :)*

* Apart from regressions that can be avoided.

Still not quite decided on the custom callback support being added here, but I'll soon start requiring 8.8 in my projects, so I guess we can just up it to that then.

My time for open source stuff is really limited at the moment... sorry

There was a patch for D9 posted at https://www.drupal.org/project/flag/issues/3146500

May we need add core_version_requirement: ^8 || ^9 to

flag/modules/flag_count/flag_count.info.yml

name: Flag Count
   description: Provides an example on how to display flag counts
   core: 8.x
+ core_version_requirement: ^8 || ^9
   type: module
   dependencies:
   - flag:flag
   - drupal:views
   - drupal:node
   package: Flags

and flag/modules/flag_bookmark/flag_bookmark.info.yml

name: Flag Bookmark
   description: Provides an example bookmark flag and supporting views.
   core: 8.x
+core_version_requirement: ^8 || ^9
   type: module
   dependencies:
   - flag:flag
   - drupal:views
   - drupal:node
   package: Flags

and flag/modules/flag_follower/flag_follower.info.yml

name: Flag Follower
description: Provides an example user flag and supporting views.
core: 8.x
+core_version_requirement: ^8 || ^9
type: module
dependencies:
  - flag:flag
  - drupal:views
  - drupal:node
package: Flags

My only doubt is about module's composer.json file, the Drupal 9 guidelines say "Having a composer.json file is not required for Drupal 9 compatibility. A Drupal 9 compatible info.yml file is required and is enough. If your project does have a composer.json file, having a drupal/core version requirement is also not required for Drupal 9 compatibility. It is better not to provide a drupal/core version requirement in composer.json because Drupal's composer facade will generate the appropriate metadata based on the info.yml file. If for some reason you do need to have a drupal/core version requirement in your composer.json file's require section, then it needs to be Drupal 9 compatible."
I tryed to remove it and now "Upgrade status" module report, says there are no problem at all. But I don't think mine wase a good practice, at all.

Hi @AmeDSL
Updated patch as you suggested please review.

Hi All,

When I tried to install the module version 8.x-4.x-dev in drupal 9 with including the patch at #32 through composer require command it got failed. It was failing because in composer.json there is dependency on drupal core of version ~8.7. There was a error of requirement failing to install the module in drupal 9, yet it was installed successfully in drupal 8.9.

So Adding the patch to make module compatible with D9.

Please review.

Just remove the composer.json, it's not needed.

And it's not possible to make a module Drupal 9 compatible with a patch, this needs to be committed first.

1. +++ b/flag.info.yml
   @@ -1,6 +1,7 @@
    description: Create customized flags that users can set on entities.
    core: 8.x
   +core_version_requirement: ^8 || ^9
    type: module
   

   Drupal 8.7 is no longer supported, so lets just require ^8.8 || ^9 now.

2. +++ b/src/FlagLinkBuilder.php
   @@ -7,7 +7,17 @@ use Drupal\Core\Entity\EntityTypeManagerInterface;
     */
   -class FlagLinkBuilder implements FlagLinkBuilderInterface {
   +// @codingStandardsIgnoreStart
   +// @todo remove this BC layer once support for Drupal 8.7 is sunsetted
   +if (interface_exists('\Drupal\Core\Security\TrustedCallbackInterface')) {
   +  interface TrustedCallbackInterface extends \Drupal\Core\Security\TrustedCallbackInterface{};
   +  }
   +  else {
   +    interface TrustedCallbackInterface{}
   +  }
   +// @codingStandardsIgnoreStop
   +
   +class FlagLinkBuilder implements FlagLinkBuilderInterface, TrustedCallbackInterface {
   

   Then we can drop this.

@Berdir -- Changes made as per your suggestion.

Please review and suggest me if any correction required.

Hi @meet_bhanvadia
Please add inderdiff its good for review the patch.

Testing issue fixes.

Adding interdiff between patch 32 &39 and patch 33 & 39.

Please review!!

Sorry for the inconvenience!! Interdiff added.

+++ b/flag.info.yml
@@ -1,6 +1,6 @@
-core: 8.x
+core_version_requirement: ^8.8 || ^9

+++ b/modules/flag_bookmark/flag_bookmark.info.yml
@@ -1,6 +1,7 @@
 description: Provides an example bookmark flag and supporting views.
 core: 8.x
+core_version_requirement: ^8 || ^9

It's a bit strange to make this inconsistent. lets require ^8.8 everywhere.

The tests are failing like this:

1) Drupal\Tests\flag_bookmark\FunctionalJavascript\FlagBookmarkUITest::testUi
PHPUnit\Framework\Exception: Fatal error: Interface 'Drupal\flag\TrustedCallbackInterface' not found in /var/www/html/modules/contrib/flag/src/FlagLinkBuilder.php on line 10

So we need to add the proper use statement now.

@Berdir - Made all consistent with 8.8 and tried to fix the test issue.

Great. Committed this to make it easier to test.

I did add a D9 test, results here: https://www.drupal.org/pift-ci-job/1721258

As you can see, all tests are still failing, because there are still test modules that are not D9 compatible, so we have to fix that too. But you can now upload patches and run tests against D9.

@Berdir - Fix the test issues for D9.

After applying patch #43 and #48, I am using it in core 9.0.0. uptill now there is no issue/problem.
If any issue or problem down the line will inform.

Thanks. @meet_bhanvadia and @Berdir

Patch #43 & #48 works for me on Drupal 9.0.0.

As you can see, all tests are still failing, because there are still test modules that are not D9 compatible, so we have to fix that too.

Is that the only reason this issue is still open?

If so, the patch in #48 is correct, and I just retested it and as you can see the tests all pass in D9 and D8 now ...

RTBC +1

You can remove the core key - test modules don't need a version requirement.

True but only with 8.8.2 or so, I'd rather clean this up at a later time. We've had some cases where other contrib modules that still want to be 8.7 compatible and had an integration test with a module that used this feature resulted in having all their tests explode on that as Drupal 8.7 is very unhappy if you have a module in the file system without a core key, even if you don't enable it. We can just remove it when we make it D10 compatible.

Oh wait, confused. that was with neither core nor core_version_requirement. The core key is being removed here in those test modules?

Committed.