Problem/Motivation

It is possible to substitute a module's service with an alternative representation. PHPUnit apparently does this a lot to create mock services. This should be possible for FlagService, but that class currently does not have an interface.

Proposed resolution

Create an interface for FlagService.

Remaining tasks

Create patch.

User interface changes

None.

API changes

No changes, only the addition of the interface.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107’s picture

Assigned: Unassigned » martin107

I am fan of unit testing, this lays the foundations.

martin107’s picture

Assigned: martin107 » Unassigned
Status: Active » Needs review
FileSize
11.01 KB

Simple cut and paste exercise.

I wanted to leave both FlagService and FlagServiceInterface in good shape - so I have taken some liberties :-

1) If I were to create an initial template using a drush generate:service command then I notice it adds an @package token so I have done that here.
2) I have reordered the use statements into alphabetical order.

joshi.rohit100’s picture

FileSize
12.12 KB
1.01 KB

With some changes also I dont know what should be done with testcases. I also found that in ActionLinkTypeBase, we have declared FlagService is unused but we have declared it.

martin107’s picture

FileSize
12.23 KB
823 bytes

Thanks for spotting that.

What follows is just minor documentation tidy-ups.

joachim’s picture

Status: Needs review » Needs work

@package is very rarely used in core, and doesn't feature at all on https://www.drupal.org/coding-standards/docs.

Also, unfortunately this has clashed with #2471435: params of FlagService::getFlagging() should match getFlaggings().

The last submitted patch, 4: interface-2475579-4.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
12.58 KB

1) reroll
2) removed @package

joachim’s picture

Status: Needs review » Fixed

> I also found that in ActionLinkTypeBase, we have declared FlagService is unused but we have declared it.

This doesn't seem to have been taken care of -- which is perfectly fine, as it doesn't really belong in this issue. I'll file a side-issue: #2477091: ActionLinkTypeBase imports classes that it doesn't use.

Committed. Thanks everyone!

  • joachim committed 60b2745 on 8.x-4.x authored by martin107
    Issue #2475579 by martin107, joshi.rohit100: Added interface for...

Status: Fixed » Closed (fixed)

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