Problem/Motivation

Remove D12 deprecated code from user and basic_auth module

Steps to reproduce

Proposed resolution

Remove D12 deprecated code from user module

Migration plugins will be removed with #3522601: [meta] Tasks to remove Migrate Drupal UI module / #3522602: [meta] Tasks to remove Migrate Drupal module

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3573870

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

smustgrave created an issue. See original summary.

smustgrave’s picture

smustgrave commented
Assigned: Unassigned » smustgrave

smustgrave opened merge request !14838

smustgrave closed merge request !14838

smustgrave opened merge request !14841

smustgrave’s picture

smustgrave commented
Status: Active » Needs review

Okay this one needs serious review, some of it I was not sure about

dcam changed the visibility of the branch main to hidden.

dcam’s picture

dcam commented
Related issues: +#3427298: Deprecate UserAuthInterface

I left a few comments on the MR. One is just a note. One is an unimportant optimization to consider.

@smustgrave I asked in Slack if you knew why UserAuthInterface isn't deprecated. Turns out that I misread the CR. It says that it will be deprecated, not that it is. There's an open issue for it, #3427298: Deprecate UserAuthInterface. I'll add it as a related issue.

Will the deprecated migrate process plugins be removed later? We night need to add that info to the Proposed Resolution so no one wonders.

Setting to Needs Work to consider my comments.

The MR looks really good. Uses of the deprecated classes and functions are removed everywhere. All deprecations are removed, aside from the migrate process plugins which I already mentioned.

dcam’s picture

dcam commented
Status: Needs review » Needs work
smustgrave’s picture

smustgrave commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Responded to the comments best I can. Thanks again for the great reviews!!

smustgrave’s picture

smustgrave commented
Title: Remove deprecated code from user module » Remove deprecated code from user and basic_auth module
Issue summary: View changes

Basic_auth had 1 simple deprecation that actually broke a test in user, but the test is removed in this MR so I just combined the two.

dcam’s picture

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

The additional change to Basic Auth is pretty simple and in-line with many of the other changes that happened to the User module. There are no other deprecations in Basic Auth that I could find.

My feedback on this one has been addressed. LGTM.

catch’s picture

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

One comment on the MR - looks like we're losing unit test coverage.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Tried my best at converting it. Super some names and descriptions will need to be tweaked.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I had one comment for the edited test. I'm setting the status to Needs Work for that.

Don't worry too much about the TestAuthenticateWith*Password() tests. I'm positive they're all bad tests. See #3570206: UserAuthTest password test audit.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Thanks!

dcam’s picture

dcam commented
Status: Needs review » Needs work

I'm sorry @smustgrave. I steered you wrong about those tests. But I think the suggestion I provided will sort it out. Please look it over.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Fixed up the test. Not sure you meant to do a createStub in a createStub?

dcam’s picture

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

Not sure you meant to do a createStub in a createStub?

Ugh. No. I was working exclusively in the MR comment text editor with no highlighting or testing of my own suggestions. I should know better. Thank you for sorting that out.

I think we're good here now.

catch’s picture

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

Needs a rebase.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Rebased, it was a conflict in a now deleted migration file.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Reviewed & tested by the community » Needs work

Needs works because of merge conflicts.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Rebased

smustgrave’s picture

smustgrave commented

Rebased

catch’s picture

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

Needs another rebase unfortunately.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Rebased, not sure why but gitlab is showing it unmergable but it applied cleanly

dcam’s picture

dcam commented

The same thing happened to one of my issues yesterday.

  • catch committed 9304d695 on main 
    task: #3573870 Remove deprecated code from user and basic_auth module...

catch closed merge request !14841

catch’s picture

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

Committed/pushed to main, thanks!

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

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

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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