[9.2] Remove jQuery dependency from the once feature

Looks ok to me

I made a Merge request with the Drupal 10 compatibility fixes.

The main change is the new once library → was introduced in Drupal 9.2.
In Drupal 10, the old jQuery Once Library is no longer available.

@lonalore: Can you please open a new issue for the jQuery.once changes? Also, it would help if you would leave out the JavaScript coding standards changes, and just made the changes to use the once library instead of jQuery.once. Coding standards should also be handled in a separate issue, as that will make the changes easier to review and manage.

#2 is not acceptable to me because this module can't support 8.8.2 and 10 simultaneously. For example with the once library changes - that will not work in D8 at all. I would prefer not to change the core version dependency to include D10 until this module is actually compatible with D10, and that means the underlying votingapi module also needs to be compatible with D10 first. That isn't the case yet.

Created patch for making votingapi_widgets to D10 compatible, Module showing compatible in Upgrade_status and I have also checked with Drupal-check which shows false positive issues which are ignorable..and also fixed the console issues related to core/once..functionality is working fine as expected in D10...kindly review and make it RTBC for D10 release..

I have tested patch #11 in drupal 9.5.7 and 10.0.3. Patch applied cleanly and functionality is working as expected, also making this module D10 compatible. So, moving this to RTBC.

@TR What we are missing here? Functionality is working fine and also there are not any issues in upgrade status and drupal check and also votingapi is on latest D10 version, what we are missing to do it compatible..??

@TR could you please consider to validate patch #11 to commit a D10 compatible release?

There is this error:

Entity queries must explicitly set whether the query should be access checked or not.

which happens in the VotingApiWidgetBase.php

Added the missing accesscheck

Rolled patch against 8.x-1.0-alpha6

Rolled patch 17, updated VotingApiWidgetBase.php

Thank you to the contributors. Please @TR have a look at the patches a commit a D10 compatible release.

The new once function messes up with the fivestars widgets (and possibly with the other types too), it seems it only runs once (which is expected), but because of this, the JS will not execute again and the widget will not render the fivestars after an ajax submit, is this intentional? Attached a video that explains the weird bug. I fixed it by removing the once completely, but I'm not sure if this is optimal

Patch 25 tested on env. Drupal 10.1.2 - PHP 8.1

Checking patch composer.json...
Checking patch js/fivestars.js...
Checking patch js/like.js...
Checking patch js/useful.js...
Checking patch src/Plugin/VotingApiWidgetBase.php...
error: while searching for:
      $query->condition('timestamp', time() - $timestamp_offset, '>=');
    }

    $votes = $query->execute();
    if ($votes && count($votes) > 0) {
      $vote = $storage->load(array_shift($votes));
    }

error: patch failed: src/Plugin/VotingApiWidgetBase.php:218
error: src/Plugin/VotingApiWidgetBase.php: patch does not apply
Checking patch votingapi_widgets.info.yml...
Checking patch votingapi_widgets.libraries.yml...

Thanks @yustinTR, maybe you can include an interdiff with the patch? https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... →

Also, shouldn't the target branch be version 8.x-1.x-dev?

I edited the patch so it works for the 8.x-1.x-dev version, the previous patch was ment for the alpha-6 version. For the interdiff the changes are to big so i got a error while create a interdif.

Fix some spacing issues & created a interdiff

added some once annotations

Hi @yustinTR for patch, #30 patch looks good to me, applied successfully and test changes on local it is now compatible and working fine for me. Hence green flag to RTBC+1...

This hunk is not needed because the accessCheck() issue was fixed in 📌 Drupal 10 add method calls to accessCheck() in getEntityForVoting() Fixed

diff --git a/src/Plugin/VotingApiWidgetBase.php b/src/Plugin/VotingApiWidgetBase.php
index 0a32db7..187600e 100644
--- a/src/Plugin/VotingApiWidgetBase.php
+++ b/src/Plugin/VotingApiWidgetBase.php
@@ -223,9 +223,9 @@ abstract class VotingApiWidgetBase extends PluginBase implements VotingApiWidget
         $query->condition('timestamp', time() - $timestamp_offset, '>=');
       }
 
-      $votes = $query->execute();
+      $votes = $query->accessCheck(FALSE)->execute();
       if ($votes && count($votes) > 0) {
-        $vote = $storage->load(array_pop($votes));
+        $vote = $storage->load(array_shift($votes));
       }
     }

I rewrote the issue summary to reflect that this patch should be restricted to just the jQuery.once changes.

The patch in #30 is looking good except that the hunk I mentioned in #32 needs to be removed and the core version requirements in the composer.json and the votingapi_widgets.info.yml need to be changed to drop D8 support and raise the minimum to D9.2, because as I said above these jQuery changes will NOT work in D8 or in versions of D9 less than 9.2 as per the change record.

-      $this.find('select').once('processed').barrating('show', options);
+      $(once('processed', $this.find('select'))).barrating('show', options);

Is this (and the other changes to js) correct? From the Change Record, I would think it should look like:

-      $this.find('select').once('processed').barrating('show', options);
+      $(once('processed', 'select', this)).barrating('show', options);

thnx @TR. The upgrade status module suggested that i needed to change that accescheck before, because the original issue was for the alpha-6 branche. but i have fixed the things you said in this patch

The last submitted patch, 35: votingapi_widgets_D10_compatible_35.patch, failed testing. View results →

+++ b/composer.json
@@ -9,7 +9,7 @@
-        "drupal/core": "^8.8.2 || ^9",
+        "drupal/core": "^8.8.2 || ^9 || ^10",

This is not the same as the change in the .info.yml, 8.8.2 should be removed

Updated patch #35 by addressing #38, please review it.

Thanks!

Looks great.

The last submitted patch, 39: votingapi_widgets_D10_compatible_39.patch, failed testing. View results →

My comments from #34 were never addressed and the changes to the module dependencies are still wrong because the once library is not available before D9.2.

Here is a re-roll. Test should be "Build Successful" which Drupal CI still considers a failure but is not - that just means this module has no tests. Drupal CI will still check that the patch can be applied and will check for syntax and coding standards errors.

The last submitted patch, 42: 3333559-42.patch, failed testing. View results →

As I said, "Build Successful" is good.

I need some people to apply patch #42 and test it on their site. If it works properly then I will commit the patch.

@richgerdes Can you test the patch? That's what we need here.

I tried latest dev and patched it with #42, which worked with 9.5.11 as far as I tested it. Upgrade status module shows 100%. But upgrade fails:

composer update --with-all-dependencies
Gathering patches for root package.
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - drupal/votingapi_widgets dev-1.x requires drupal/core ^8.8.2 || ^9 -> found drupal/core[8.8.2, ..., 8.9.20, 9.0.0, ..., 9.5.11] but it conflicts with your root composer.json require (^10).
    - drupal/votingapi_widgets 1.x-dev is an alias of drupal/votingapi_widgets dev-1.x and thus requires it to be installed too.
    - Root composer.json requires drupal/votingapi_widgets 1.x-dev@dev -> satisfiable by drupal/votingapi_widgets[1.x-dev (alias of dev-1.x)].

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.

But i can see:

cat ./web/modules/contrib/votingapi_widgets/votingapi_widgets.info.yml
name: Votingapi Widgets
type: module
description: 'Voting API Widgets.'
package: Voting
core_version_requirement: ^9.2 || ^10

dependencies:
  - votingapi:votingapi
  - drupal:field

This is because there is a composer.json in the source code, that needs to be updated to the same version constraints as in the .info.yml

This is because there is a composer.json in the source code, that needs to be updated to the same version constraints as in the .info.yml

Both the votingapi_widgets.info.yml and the composer.json have the same core version constraints. Neither specifies support for D10 yet, because issues like this prevent this module from being compatible with D10 at the moment. That is why it would be nice to get some reviews, so we can move forward.

my installation seems broken, because i can't find 8.8.2:

Fileserver:/srv/www/drupal/web/modules/contrib # grep -i -r '8.8.2' votingapi_widgets/*
Fileserver:/srv/www/drupal/web/modules/contrib # grep -i -r '9.2' votingapi_widgets/*
votingapi_widgets/composer.json:        "drupal/core": "^9.2 || ^10",
votingapi_widgets/votingapi_widgets.info.yml:core_version_requirement: ^9.2 || ^10

i got it running, https://github.com/mglaman/composer-drupal-lenient did the trick.

Hi @TR, we also need to update this module on one of the projects we support, so this week we should let you know how it went using the branch https://git.drupalcode.org/issue/votingapi_widgets-3333559/-/compare/8.x...

@rafal.sereda: Do you have an update yet?

ciprian.stavovei → made their first commit to this issue’s fork.

the patch in #42 applied successfully to dev for me (the MR did not) and the upgrade_status tests for D10 all passed.

It appears to work in my local site.

I don't know what #55 is supposed to do. I has all sorts of wrong things in it and it doesn't address the JavaScript issues which are the entire reason for this issue.

The patch in question remains #42. @loze is the only one who has reviewed it - thank you. I will commit this next time I have a chance to sit down and make sure everything is still up to date. Probably this weekend.

Thanks, for the patch.

@TR: please commit a release with patch #42. Thank you very much.

This project is no more supported. Can the maintainers explain why?

Did you review the patch? We've had exactly 1 review, less than a month ago.

@TR Patch #42 works for me on our dev site and is passing PHPStan with Drupal Core at 10.2.1

Patch #42 works, thanks!

@TR: the #42 patch has been reviewed & tested by the community. Please commit a release with it.

There is a security related update for drupal core available, but not for 9.x
Can we please have a release now?
My dev site works with the patch, with the already posted three issues.

@DrupalBubb: awaiting an answser from @TR and @ipwa to give @rajab-natshah co-maintainer credentials.
https://www.drupal.org/project/votingapi_widgets/issues/3414710 📌 Offering to co-maintain Voting API Widgets Needs review

Try using the mglaman/composer-drupal-lenient composer plugin as a TEMP selution to update to Drupal 10 for security reasons

You could follow up with the README file.

composer config minimum-stability dev
composer require mglaman/composer-drupal-lenient
composer config --merge --json extra.drupal-lenient.allowed-list '["drupal/votingapi_widgets"]'

and the following patch

      "drupal/votingapi_widgets": {
        "Issue #3333559: [9.2] Remove jQuery dependency from the once feature":
        "https://www.drupal.org/files/issues/2023-10-21/3333559-42.patch"
     }

successfully updated a drupal site using MR2 - thanks!

I still would prefer a buggy dev version. No version means no future.

what's the situation with a D10 version will a dev version be released soon?

@stevieb: because of the complexity of this issue, the maintainers need support from Drupalistas contributing on Drupal core. I think in particular about Mike Herchel → to fix this issue. Joris (@borisson_), who contributed to this issue, knows him personally and could hopefully get in touch with him and ask for help.

TR → changed the visibility of the branch 3333559-d10-compatibility-upgrade to hidden.

TR → changed the visibility of the branch 2.0.x to hidden.

TR → changed the visibility of the branch 3333559-v2 to hidden.

Committed to 8.x-1.x and 2.0.x.

Please help out with 📌 JavaScript coding standards Active . I strongly suggest moving to the 2.0.x branch at this point, but I will probably make one more last release on 8.x-1.x once the JavaScript issue is finished.

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