Problem/Motivation

Right now the access function in both BlockUser & UnblockUser classes is identical. This is unnecessary code duplication as the two classes handle the same type of functionality.

Steps to reproduce

N/A

Proposed resolution

Add a new abstract class, ChangeUserStatusBase and have both BlockUser and UnblockUser extend that. There is already precedent for this in the user module with ChangeUserStatusBase.

Remaining tasks

  • Agree on approach
  • Review
  • Commit
  • CR?

User interface changes

None

API changes

New abstract class ChangeUserStatusBase
Access function removed from BlockUser & UnblockUser
BlockUser & UnblockUser now extend ChangeUserStatusBase

Data model changes

None

Release notes snippet

TODO

CommentFileSizeAuthor
#7 3260036-7.patch3.16 KBdanielveza

Issue fork drupal-3260036

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

DanielVeza created an issue. See original summary.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Issue summary: View changes
danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Active » Needs review

I pushed a commit for this but I can't see the command. Ready for review so tests run

DanielVeza opened merge request !1705

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

LGTM.

quietone’s picture

quietone commented
Version: 9.4.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Needs work

This needs to be applied to 10.0.x first and can we have a 9.5.x version as well. Thanks!

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
StatusFileSize
new 3260036-7.patch3.16 KB

The existing MR applies to both 10.x & 9.5 :)

Attaching a patch of the diff from that MR so it can be tested against 10.x & 9.5

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs work » Needs review

I think this can go back to NR

acbramley’s picture

acbramley commented
Status: Needs review » Reviewed & tested by the community

Patch changes match the MR changes, back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3260036-7.patch, failed testing. View results

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs work » Reviewed & tested by the community

I think thats a random fail. RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

This change creates more API that needs to be maintained. At the moment the plugins \Drupal\user\Plugin\Action\UnblockUser and \Drupal\user\Plugin\Action\BlockUser are not API - since they are concrete plugins. Adding Drupal\user\Plugin\Action\ChangeUserStatusBase gives us a new abstract class to maintain. I'm not sure there are real benefits here. Also I'm not sure there is much call for this base class in contrib.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Yeah I'm also not sure about this, it doesn't meet the rule of three and there's only a total of three lines of code being removed, the diffstat is almost equal.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record

Seems there is more discussion to be had based on #12 and #13.

If this does go through it will need a change record I believe.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented

I'm OP - I think we should close this one based on feedback from #12 & 13. Doesn't need to block up the issue queue :)

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs work » Closed (won't fix)

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.