Problem/Motivation

There is one query to the Database whose value ends up sent as "null" and Drupal Database connection class throws a warning:

Deprecated function: addcslashes(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\Core\Database\Connection->escapeLike() (line 1525 of core/lib/Drupal/Core/Database/Connection.php).
Drupal\Core\Database\Connection->escapeLike(NULL) (Line: 420)
Drupal\Core\Database\Query\Select->escapeLike(NULL) (Line: 118)
...

Steps to reproduce

Using fivestar module with PHP8.1

Proposed resolution

Check for empty variables with "empty" function or similar instead of boolean "!" operator.

Remaining tasks

-

User interface changes

-

API changes

-

Data model changes

-

Issue fork fivestar-3324052

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

dpavon created an issue. See original summary.

dpavon opened merge request !14

dpavon’s picture

dpavon commented

Hi, this is the first time I commit through the UI, so take it with a grain of salt.

As for the query / code change itself, I would assume we don't want to perform the queries if the values are NULL, since the warning would then show up.

If, however, the query needs to be run with some of these parameters as optional, then instead we will need to replace the "NULL" values for empty strings or don't add the property so that on PHP8.1 we don't get the warning.

I will continue testing the module to gather this context that I lack right now. I guess we'd normally need entity_type at all times, but entity_id could be optional. However, contextually wise I'd prefer to have both so we can properly evaluate if the user can re-vote an entity. I don't see the value for now on having empty values intentionally.

dpavon’s picture

dpavon commented

Hmm, I checked a bit more the code, and the lines that review that entity_id and entity_type are not empty are far below and return TRUE.

I took the change to refactor some more lines and commited anew. I'll re-test.

o'briat’s picture

o'briat
Primary language French
Location Nantes
commented

Is it related to the Drupal 10 compatibility?
Shall I merge this commit into the Automated Drupal 10 compatibility fixes MR ?

heddn made their first commit to this issue’s fork.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

This fixes the underlying issue by always providing the context information to the render element.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Active » Needs review
tr’s picture

tr commented
Status: Needs review » Needs work

The accessCheck issue is #3391088: Error: Entity queries must explicitly set whether the query should be access checked or not and that fix should be handled in the issue that describes that problem. It should NOT be mixed in with this unrelated issue. We already have far too many issues that all conflict with each other because they all mix in random unrelated code changes.

tr’s picture

tr commented

Yes, the correct way to fix this error from addcslashes() is to make sure the variables used always have a value.

There are at least five other issues that make many of these same changes surrounding the userCanVote() method. And part of the problem is that the changes in Voting API from D7 to D8, namely the renaming of content_id/content_type to entity_id/entity_type do not seem to be fully reflected in the current Fivestar code.

But if these changes are to be made, they shouldn't be made in isolation - all those other issues need to be looked at and the comments read to ensure this is being changed in the right way and not in a way that we already know doesn't work. Likewise it would be nice to give credit to the people who did the work on those other issues, rather than just throwing away their contributions. In fact IMO it's highly likely that this addcslashes() issue is just a side-effect of an already-known problem and will disappear as a result of fixing those other issues.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Rebased MR on HEAD and changed from content_* to entity_* wording as per #10.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Needs review

And now with test coverage.

wxman’s picture

wxman commented

I'm getting nothing but the error on any page that has Fivestar. I'm using the alpha4 release, but I don't get which MR or patch I should be using? It's a bit confusing. Thanks

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

MR 14 (https://git.drupalcode.org/project/fivestar/-/merge_requests/14.patch) should fix the issue.

wxman’s picture

wxman commented

@heddn Sorry to be a pain. I added the patch and ran this:

@server2:~/newsite$ composer require 'drupal/fivestar:^1.0@alpha'
./composer.json has been updated
Running composer update drupal/fivestar
Gathering patches for root package.
Removing package drupal/fivestar so that it can be re-installed and re-patched.
  - Removing drupal/fivestar (1.0.0-alpha4)
Deleting web/modules/contrib/fivestar - deleted
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Installing drupal/fivestar (1.0.0-alpha4): Extracting archive
  - Applying patches for drupal/fivestar
    https://git.drupalcode.org/project/fivestar/-/merge_requests/14.patch (#3324052 Deprecated function addcslashes)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/fivestar/-/merge_requests/14.patch
    https://www.drupal.org/files/issues/2023-10-02/fivestar-access-check-3391088-2.patch (#3391088 Entity queries must explicitly set)

My composer.json looks like this in the extra section:

    "extra": {
        "patches": {
            "drupal/fivestar": {
                "#3324052 Deprecated function addcslashes": "https://git.drupalcode.org/project/fivestar/-/merge_requests/14.patch",
                "#3391088 Entity queries must explicitly set": "https://www.drupal.org/files/issues/2023-10-02/fivestar-access-check-3391088-2.patch"
                }
        },

Sometimes I miss the obvious.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Fixed

Let's just merge this and tag a new release.

  • heddn committed 83df5c02 on 8.x-1.x 
    Issue #3324052 by heddn, dpavon, TR, wxman: Deprecated function...
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

https://www.drupal.org/project/fivestar/releases/8.x-1.0-alpha5

wxman’s picture

wxman commented

@heddn when you update the branch like that do we leave the patch in as is, or remove it and just run composer require 'drupal/fivestar:^1.0@alpha'

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Normally you would leave the patch file in, but now you can just `composer update` and get alpha5 and remove all patches.

wxman’s picture

wxman commented
Status: Fixed » Needs review

@heddn That did it! No errors, and I can edit the star ratings. You folks are great.

ivnish’s picture

ivnish
he/him
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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