Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sathish.redcrackle created an issue. See original summary.

sathish.redcrackle’s picture

Status: Active » Needs review
idebr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
rpayanm’s picture

rpayanm’s picture

rpayanm’s picture

Issue tags: -Needs reroll
andralex’s picture

I've tested the patch and generally, it works for me as it is fixes database queries deprecations.

What I've found that there is another issue for removal of deprecated functions https://www.drupal.org/project/votingapi/issues/3042715. It is doing almost the same but also fixes other deprecations. Probably we could close this issue or make it as a child.

Another concern is about coding standards. Here are 42 coding standards messages. It would be great if we could fix them.

TR’s picture

#3042715: Drupal 9 Deprecated Code Report should be narrowed because it is trying to do too many things at once - that makes it hard to review and also makes it conflict with other open issues like this one.

For the replacement of db_ functions, the patch in #6 is better than the solution in #3042715: Drupal 9 Deprecated Code Report because #6 does dependency injection. So we should keep this issue open and get it resolved here and not in #3042715: Drupal 9 Deprecated Code Report

Coding standards should be handled in a separate issue. We should focus on making sure that the patch in #6 does not introduce NEW coding standards problems, however it should not try to fix EXISTING coding standards problems.

TR’s picture

Here's a re-roll of #6 that applies to the current HEAD.

I had to make two changes:

  1. I removed all the changes to votingapi.drush.inc, because they were already done as part of #2858130: drush vcalc is not working
  2. I removed the changes to hook_uninstall(), as it is WRONG to explicitly delete config in hook_uninstall(), so instead of changing the db_* functions we should just be removing hook_uninstall() entirely. But that should be a separate issue! (I opened #3134574: Remove hook_uninstall() for this.)
TR’s picture

Looking at the test results, I see that there are 6 NEW coding standards violations.

These are because the patch in #6 add some unused "use" statements - 6 of them in fact. These aren't needed for the patch, and aren't needed for the code, and the above comments don't say anything about why they were added. They are in fact unneeded.

So here's a re-roll of #10 that removes these unneeded "use" statements. We don't wan to be adding any new coding standards violations.

JordiK’s picture

Patch in #11 works well - RTBC.

JordiK’s picture

Status: Needs review » Reviewed & tested by the community

  • pifagor committed ff52ac3 on 8.x-3.x authored by TR
    Issue #2873325 by rpayanm, TR, sathish.redcrackle, JordiK, idebr,...
pifagor’s picture

Status: Reviewed & tested by the community » Fixed

Done

Status: Fixed » Closed (fixed)

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