Well I think it has the same root cause, and regardless I am not going to cross-post everything in two separate threads. Everything I said there applies here as well, and a proper fix to handling row_num that includes tests (and especially tests for special-case processing of webforms and commerce) will have to address all variations of this issue, otherwise it won't be a proper fix.
Please post this information in the existing issue 🐛 TypeError: Drupal\views_aggregator\Plugin\views\style\Table::setCell(): Argument #2 ($row_num) must be of type int, null given Active
Did not address #8.
2.0.x supports Drupal 11.
Still needs tests.
Please answer the questions I asked in #2.
To repeat, I am not working on this because I don't use Layout Builder.
And if this error goes away when you set a default value for the Fivestar field, then I'm inclined to think the problem is with Layout Builder as I explained in that other issue.
Needs tests.
"use" statements need to be alphabetical.
What about #wrapper ?
Umm, point of order.
How did you expect to get feedback for this proposal, before making this change? I never knew about this until it was committed and deployed. Apparently no one else knew about it either, because there have been no comments either pro or con.
I maintain modules that collectively are used on over 250,000 Drupal sites. I didn't know about this change until it happened. Yes, I am subscribed to all the relevant notification channels on drupal.org, and I can't possibly know about or follow every single issue queue that might affect me and the users of my modules. I do not participate in the "project" issue queue, and without checking this issue queue I apparently have no way of finding out about changes like this until they are already a done deal.
It seems to me that you should have solicited input from people like me before you made this change. Maybe via a maintainer's mailing list or some such mechanism. I *am* subscribed to the maintainer's mailing list, but I have never received any information through that channel.
The problem with this change is:
I have some project that now show TWO valid releases, rather than ONE "recommended" release (green) and one not-recommened (yellow) release which exists only for backwards compatibility. How do I distinguish between these two releases? And how to I mark one release as recommended over the other? Do I need to unpublish the older release? But the older release is still relevant for backwards compatibility, so how do I deprecate the use of the older release and encourage users to move to a current release, without having to cut them off entirely?
Sure, I don't see why not. I'm not planning on working on that, but if you want to contribute something we can get that added in.
I moved the patch into an MR to that it can be tested.
That section of code should only be called if you are using a Radioactivity field instead of a Radioactivity Reference field.
In 4.1.x, you can't use a Radioactivity field unless you've upgraded an old site from 8.x-3.x and you were using the Radioactivity field in 8.x-3.x. Using the Radioactivity field is something that is supported only for backwards compatibility, but it is not allowed on new installs.
Does this correctly describe your situation?
Rather than using a cast, I would prefer to use getValue() so that the datatype of the energy and timestamp variables is maintained. They are declared as float and integer respectively, in the database schema.
I think this is all good, but I really need a test case for this. I can write it if you can write down the specific steps needed to cause this error. I need to know how to set up the site - how to set up the fields/bundles/content types, then what actions have to be taken to get the wrong result and where does the wrong result show up? Do this for both of your cases mentioned in #11.
I have looked back through the commits, and \Drupal\votingapi\Entity\Vote *never* had its own preSave() method, even as far back as 2019 when this issue was opened.
So I'm going to assume the issue summary is just inaccurate, but I don't think I can assume that the problem doesn't exist.
I suspect this is still relevant, but it is something that someone should try to reproduce. We need specific clear directions that spell out what steps need to be taken to cause this problem. It seems that you need at least two votingapi_widget fields of different types attached to the entity, but then what steps need to be taken to make this problem appear? Where does the "remove previous votes from the user if he already voted on the same entity" happen?
Then if it's still a problem we should have a test to demonstrate the problem, and then we can talk about a solution. I'm not a fan of overriding the votingapi Vote entity like this patch does - if this is still a problem then perhaps we ought to change the votingapi module to recognize and support multiple types of votes on the same entity.
Yes, thanks for finding that. Maybe the form definition for readonly, show_own_votes, and show_results also need to be changed a bit in VotingApiFormatter? They are checkbox elements, so I think they need a #return_value of TRUE if we want to keep these config settings booleans?
Provides a global config page where the admin can choose to disable fontawesome and glyphicons from being included for cases where you are already including them in your theme (or don't want them at all).
Is this part really needed? If these are assets we need for votingapi_widgets, then we need to load them - there should be no choice. Likewise, I think the whole point of using libraries like this is that Drupal core takes care of removing the duplicates - you shouldn't have to be aware of what the theme or some other module is loading.
I think returning a form array is the preferred method in this case.
Regardless, if you return an AjaxResponse, it can't be an empty response - you have to ->addCommand() and provide the form array to the response.
Bottom line, not a bug, since we are currently using the recommended method for this simple use-case, and needs work because returning an empty AjaxResponse will not work.
So ... that means everyone who is using the 8.x-1.x and 2.0.x versions of this module has a bunch of excess rows. If that's the case, then we also need a hook_update_N() function here to clean up the database on existing sites.
I changed your MR to use the new object-oriented hooks, since everything else in this module and Voting API has already been changed to OO hooks.
tr → made their first commit to this issue’s fork.
That comment, and the code implementing that comment, were added by #2651098: Emit javascript being triggered by ajax → to solve a specific problem.
I have no objections to changing this, if you want to write a patch to do that.
But it's important to me that we don't regress, so please read that previous issue and propose something that will work in your case without causing the same problem described in that other issue.
Well that made it worse, to put that back in.
Status of this issue is that the tests are correct now, but both the anonymous and authenticated roles are still being deleted when the module is uninstalled. There is something happening in core Drupal and it's going to take some research to figure out how to define the field permissions so that roles (which we don't use anywhere) don't get deleted.
Reverted "solution proposed by @shifali baghel in #3265224-19" because that didn't fix the problem.
This should leave nothing but the fixes to the tests, which I may just commit because the fixes need to be made even if we can't solve this issue immediately.
composer require --dev drupal/tr_rulez:dev-2.0.x will work.
The dependency on Rules -dev was already changed, but there is no stable release with that change yet.
The ConfigFormBase constructor was changed starting in Drupal core 10.2, so the HTML Mail configuration form won't work in any currently supported version of Drupal core.
🌱 The future of the HTML Mail module Needs review
The existing test fail with these changes because the tests were not changed to accommodate the alterations made by the MR.
Also, there are a bunch of phpcs errors because method parameters are not documented.
Both these things need to be fixed before we can progress on this issue.
I re-rolled the patch for 4.0.x and put it into an MR for testing.
tr → created an issue.
@poker.ca
This issue is about the error:
TypeError: Drupal\views_aggregator\Plugin\views\style\Table::getCellRaw(): Return value must be of type string, false returned
MR!27 solves that specific error, which results from the situations described in #3. That's a very specific issue, which has a very specific cause, and the MR solves that very specific issue.
The patch provided by @scampbell1 does NOT solve the very specific issue that is described in the issue summary above.
Regardless, what I said in reply to @scampbell1 in #21 is still true:
it's a different problem and you should open a different issue if you can reproduce the error on a clean site.
So, @poker.ca , can you reproduce your error on a clean site? And, as I asked in #21,
[did] you update your version of the module without running the update hook
?
@poker.ca: You clearly have a DIFFERENT problem. So open a NEW issue for your problem, and provide instructions for how to reproduce that problem on a clean site.
The $group_aggregation_results variable should ALWAYS be an integer. If it isn't, ON A CLEAN SITE, then there is a problem with this module that needs to be solved (in a different issue, because that's not the problem the OP raised). This can't be solved without community input as to the exact conditions that cause this problem.
$group_aggregation_results is defined in the schema as an integer, and is treated in the code as an integer, and should always be an integer. The only reason I know of where it would not be an integer is if the module was updated from an older version (where this variable was sometimes a string) and the update hooks were NOT run. That would be a user error, not an error with this module. Again, the only way to determine this is if you provide instructions for reproducing this error on a clean site.
The method described in #339554-6: Reset Votes → was actually implemented in a drush command, back in 2010.
If that does not work, in the current version of Drupal/VotingAPI, then please open a new issue.
Also type hint everything in the storage classes.
Committed. Thanks.
Sounds good to me. Thanks for your contribution.
I created a MR for the 4.0.x branch.
This will not be put into the 8.x-3.x branch.
Note that the tests fail (in both the new MR and the old MR) because the new uuid field is not being initialized. This would not happen if VoteResult simply inherited the uuid field from ContentEntityBase.
Like I said, instead of defining our own new base field, we should just add the uuid entity key and call parent::baseFieldDefinitions(); so that ContentEntityBase will define and handle the uuid field.
This fix is needed before we can release 4.0.0.
tr → changed the visibility of the branch 2965603-add-support-for to hidden.
tr → changed the visibility of the branch 3510555-incorrect-variable-type to hidden.
The MR was created against 8.x-3.x instead of 4.0.x.
This change isn't going to be put into 8.x-3.x
I will create a new branch in the MR for 4.0.x.
Do you think this should be committed? Or should this stay here in the issue queue as a patch for those who might need it? Only a few percent of the users of this module are still on D6, and it is so very out-of-date that migrating from D6->D11 is more work IMO than starting with a fresh D11 site.
I'm not against committing this, because you've already done the work and some may find it useful. But be aware that I don't want people to get the impression that this is supported code and that they can get help for it here in the issue queue. This has no test cases and cannot be tested on GitLab so there is no way to show it works or to detect problems with the code. I cannot support this code, at all, because Drupal 6, 7, 8, and 9 core are all unsupported.
Shouldn't there also be a migrations/state/votingapi_migrate_drupal.yml file?
I would like to help get the Voting API to tagged release that is compatible with Drupal 11
Then please help with the existing issues in the issue queue. The big blocker is the needed changes to the structures of the entities defined by this module. Those changes can only be made in a major point release, so there won't be a release until this work is done.
- Please open a MR. Patches can no longer be tested by drupal.org.
- Once you have done that please hide all the obsolete patches.
🐛 Error on login Closed: duplicate
I don't know what #22 is, but the patch is just wrong and the reported error has nothing to do with this issue. hook_help() is typed to return ?string, which means string or NULL, exactly as core defines it. There is nothing wrong with the rules implementation of hook_help().
@jeremey1606: Are you planning on doing something here?
Your patch looks correct. I will not able to get this changed until later in the week.
This was fixed by 2.2.2 - that's why 2.2.2 was released a week after 2.2.1.
https://www.drupal.org/project/honeypot/releases →
✨ Conflicting name with other element(s) Needs review
That appears to be the Rate module calling the Vote API with a missing argument. That's a bug in Rate, which was revealed the above changes.
tr → changed the visibility of the branch 3153550-vote-entity-unnecessarily to active.
It looks nice.
Unfortunately I only have limited permissions to maintain this project, and I can't edit the project page to add your module.
But I can help you update it for Voting API 4.0.x if you plan to continue to maintain and develop your project ...
If you edit *your* project page and add your project to the "Voting API" ecosystem, that will make your module easier to find because it will be linked from the Voting API project page.
I just added phpcs:ignoreFile to votingapi.api.php, so we don't have to deal with any of this any more.
Merged.
I guess I don't have a choice, VoteResultFunction plugins are too intertwined, so have to work on the typing in those at the same time it seems.
Also for related types like VoteType and VoteStorage.
VoteResultFunction plugins and manager will be handled in a separate issue.
Actually, I'm not sure what this issue is about?
The Voting API module does not define a field type.
Perhaps you're asking about some other voting module?
This is going to have to wait until I can add strict typing and a fluent interface to the Vote entity. That will prevent the error shown in the test output.
@the_g_bomb: Can you rebase the MR? I am fine with committing this patch as-is, once it is rebased. It just fell off my radar since September, because I've been doing a lot of other things (hundreds of commits and thousands of replies in issue queues).
As of about 7 Sept 2023, GitLabCI and this module have been able to run tests on MRs. It was a LOT of work on my part, collaborating with core and Drupal infrastructure maintainers, over a period of more than two years, to make this happen. And the switch from DrupalCI to GitLabCI threw a huge monkey wrench into the process.
You're welcome.
I don't appreciate criticism from those who demonstrate no inclination to contribute to Drupal or this module. If you want to be part of the solution, stop being part of the problem.
I will note that no-one thought it was worth 10 minutes of their time to put this into a patch back then when MRs were not supported.
And I will note that in the past four years no-one thought it was worth 1 hour of their time to create a test case.
As of today, this needs to go into 2.0.x. And we still need a test case to define and demonstrate the problem, as well as prove the solution. This module now has a few test cases that work, so this task is even easier than it was before.
To anyone who cares about this issue:
- Provide a test case that reproduces the problem by failing.
- Provide a MR (including the test case) that passes, proving the fix corrects the problem.
This is a community project. I am a volunteer maintainer - I do NOT work for you. If you care about this project and/or this issue, then contribute. I will not tolerate entitled users who demand that I (a volunteer) work on and fix problems that that they (paid employees) are experiencing. If you have a problem, I will provide guidance and enable the solution, but I feel no obligation to do your work for you.
OK!
Look at the test results... The new test, VotingApiFieldTest, now passes!
This test simply uses the UI to add a new voting_api_field to a content type, then configures the field through the UI.
This is testing the exact situation that @hepabolu described in the original post.
Note that the test PASSES with this MR, which includes the test and the fix.
More importantly, note that the "test only" pipeline, which includes just the test but not the fix, FAILS.
This means that the test reproduces the problem, and ALSO proves that the fix works.
I'm happy. In addition, the process of writing the test pointed out a few errors in the codebase. Those are now corrected by the MR.
The code quality has gone up drastically, and a major bug has been fixed, all thanks to one test. This is EXACTLY why I am so insistent upon adding tests to this module.
OK, here's a simple test. Might have to debug on the testbot.
Well to answer your question,
The markup for the "useful" widget (thumbs up / thumbs down) is added by js/useful.js, so it cannot be changed with css or a template. You would actually have to edit js/useful.js and swap the order of the html elements with the fa-thumbs-up and fa-thumbs-down classes. Additionally, you will have to change .eq(0) and .eq(1) and each appear in exactly one place in js/useful.js. You will have to change these two lines so that 0 becomes 1 and 1 becomes 0. That should be all.
Here's a patch that will do this on the current version of votingapi_widgets.
I put the patch from #2 into a MR against the 2.0.x branch.
This still requires a test case to ensure the css class gets added in the right place.
Just noticed, all the documentation short descriptions for these plugins are wrong - seems to have been copied from some other plugin 10 years ago when these were first written. May as well correct that here too ...
There is a lot of work that needs to be done and I would be glad to have help doing it. Writing test cases (desperately needed) / finding and reporting bugs / writing documentation / responding to the issues in the queue / reviewing patches / contributing code, etc. Anyone can do these things, however - it does not require co-maintainer privileges. I don't think you've participated in this issue queue before - is there something in particular you want to do that can't be done by opening an issue?
This issue needs an update function to fix the changed datatype for the field formatter "decimals" setting, on sites that already have this module installed with the old datatype.
Thank you.
No response from owner after 18 days. Moving to project ownership queue as the next step.