Replace all deprecated code as per the drupal standards as suggested by the maintainers.

CommentFileSizeAuthor
#8 3257479-7.patch18.15 KBmegha_kundar
#4 3093198-4.patch20.95 KBshubham.prakash
#2 3093198-2.patch20.95 KBshubham.prakash

Issue fork diff-3093198

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

shubham.prakash created an issue. See original summary.

shubham.prakash’s picture

shubham.prakash commented
Status: Active » Needs review
StatusFileSize
new 3093198-2.patch20.95 KB

Patch added for the same.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

Thank you for getting this started.

The argument order is different. First needs to be the expected value, then the actual.

shubham.prakash’s picture

shubham.prakash commented
Status: Needs work » Needs review
StatusFileSize
new 3093198-4.patch20.95 KB

The actual and expected values have been swapped.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

That looks pretty good to me.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

I was going to commit this, but it looks like it needs a reroll against 8.x-1.x.

megha_kundar’s picture

megha_kundar commented
Assigned: Unassigned » megha_kundar
megha_kundar’s picture

megha_kundar commented
Assigned: megha_kundar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3257479-7.patch18.15 KB

Status: Needs review » Needs work

The last submitted patch, 8: 3257479-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

saphemmy’s picture

saphemmy commented
Assigned: Unassigned » saphemmy
saphemmy’s picture

saphemmy commented
Title: Replace deprecated assertEqual() with assertEquals() » Replace all deprecated functions in tests
Issue summary: View changes

saphemmy opened merge request !12

saphemmy’s picture

saphemmy commented
Assigned: saphemmy » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll

Fixed all the deprecations within tests.

victoria-marina’s picture

victoria-marina commented
Assigned: Unassigned » victoria-marina

I'll review this.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Something is wrong here, the testbot didn't trigger or provide feedback...

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

This is awesome work. Very thorough and well-scoped. I found a few things but they're basically just nitpicks. The only thing really blocking commit here is the fact that, for some reason, the testbot isn't running on this issue. Beyond that, I'm also seeing pre-existing failures in HEAD (which might be why testbot isn't running, come to think of it): https://www.drupal.org/pift-ci-job/2311570

victoria-marina’s picture

victoria-marina commented
Assigned: victoria-marina » Unassigned

After the #12, all deprecations from tests were gone. I will keep observing the comments about @phenaproxima's thoughts.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The reason it doesn't work is because the module is configured to use Drupal 8.9 by default, which is no longer a valid option.

The default configurations on https://www.drupal.org/node/20492/qa need to be updated to use 9.3 or so as the core version.

That, combined with a number of bugs around merge requests, like sometimes not being able to re-test, apparently including the case when there is no (valid) default test configuration.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Actually, I see that this has been changed already, the old ones just haven't been removed yet. However, it happened after this has been pushed I assume.

I'd recommend using the stable/supported version of D9 for testing, so that it doesn't break if core changes some strings like currently the case. You can have a weekly test against 9.4 with notifications (daily is imho overkill).

So the only way to get it tested is to push a new commit to address that review or upload a patch.

saphemmy’s picture

saphemmy commented
Assigned: saphemmy » Unassigned
Status: Needs work » Needs review
victoria-marina’s picture

victoria-marina commented
Assigned: Unassigned » victoria-marina

I'll review this.

victoria-marina’s picture

victoria-marina commented
Assigned: victoria-marina » Unassigned
Status: Needs review » Reviewed & tested by the community

After the last commit, the changes suggested on #16 were applied. Everything works fine as far I can test, so its a RTBC for me.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Adjusting credit for commit.

  • phenaproxima committed b969dea on 8.x-1.x authored by saphemmy 
    Issue #3093198 by saphemmy, shubham.prakash, Megha_kundar, victoria-...
phenaproxima’s picture

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

Annd, merged into 8.x-1.x. Thanks all!

Status: Fixed » Closed (fixed)

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