Hey there,

I just downloaded the new release of Fivestar and installed it.

After some hours I checked the Watchdog and found a huge amount of notices.
It seems like you fixed one description notice (after a long time), but brought like 3-4 new ones per request.

Please code more carefully and use default arrays like the one in my patch to prevent such things.

Also the fivestar_custom_widget() function I edited in the patch is missing documentation for the new "allow_ownvote" and "allow_revote" options.

Thanks for reading.

Hope for a quick fix of this.

func0der

Comments

func0der’s picture

StatusFileSize
new1.62 KB

Status: Needs review » Needs work

The last submitted patch, 1: fivestar-notice_removal-2220139-1.patch, failed testing.

func0der’s picture

StatusFileSize
new1.49 KB

Fixed paths.

func0der’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: fivestar-notice_removal-2220139-3.patch, failed testing.

whiteph’s picture

Version: 7.x-2.0-rc3 » 7.x-2.x-dev

I'll look at this asap.

whiteph’s picture

Status: Needs work » Needs review
whiteph’s picture

Regarding "Please code more carefully" - a more sensitive person than me might take offence at your comment. As we all know, coding for Drupal has quite a steep learning curve - as does many other aspects of Drupal. I'm very happy to learn from more experienced Drupal developers, but please don't take such a condescending style. Do you think I and other FiveStar contributors are being sloppy? There's a very big difference from a developer working carefully but within the limits of his knowledge of Drupal, and one just churning out sloppy code.

Indeed, I might ask you to "Please ensure your patches posted on D.O. pass the automated tests". I've learned the hard way that patches posted on D.O. fail the automated tests if the version isn't set to the dev version - hence me changing the version in #6, so the re-test worked successfully. Now that might be an underlying problem with the FiveStar project, or a "feature" of D.O., but either way it's a nugget of knowledge that you might find useful in future.

Your patch looks innocuous to me, and I'll almost certainly apply/commit it over the coming weekend. But when I look through my watchdog report, the notices I see seem reasonable. However, I don't yet understand why your patch would fix them.

So, please describe your issue in more detail:

"a huge amount of notices" - tens, hundreds, thousands, millions, … ? Please quantify "huge". What notices? Please give examples.

"it seems like you fixed one description notice (after a long time), but brought like 3-4 new ones per request." - what was the one fixed? How long is a long time? What do you mean by "request"?

"missing documentation" - do feel free to update the documentation. Those two options are pretty straightforward.

Why will your patch solve the issue?

whiteph’s picture

Status: Needs review » Closed (cannot reproduce)

If re-opened please describe the notice messages.

I applied the patch in my test system, with no observable effects, so I didn't apply to 7.x-2.x-dev.

whiteph’s picture

Just a thought -did you run the updates?

func0der’s picture

Status: Closed (cannot reproduce) » Active

Maybe you got me wrong there.
It was not my intention to be rude, but I was annoyed by the the notices.

Whatsoever: I'm sorry.

The following is not intended to be rude, but is kept short, because it is much.

The "code more carefully" thing is something, that is not Drupal specific. It's a general thing. Default variables are used to spare all the "isset()" calls in the upcoming function body.
I did not want to hurt any feelings there, it was just the second time, a similar error ooccured in a row. And since we are using this in corporate environment we are held to stick to the code that comes from the modules directly to keep it updateable. No forks etc. in any way if we can get around it somehow.

Thanks for the tip with the dev version and the testing. Any idea, why this is happening?
What if there is no dev version?

Regarding the notices:

"a huge amount of notices" - tens, hundreds, thousands, millions, … ? Please quantify "huge". What notices? Please give examples.

Notice: Undefined index: allow_revote in fivestar_custom_widget() (Zeile 448 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).
    Notice: Undefined index: allow_ownvote in fivestar_custom_widget() (Zeile 449 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).
    Notice: Undefined index: description in fivestar_custom_widget() (Zeile 454 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).
    Notice: Undefined index: stars in fivestar_custom_widget() (Zeile 445 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).
    Notice: Undefined index: allow_revote in fivestar_custom_widget() (Zeile 448 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).
    Notice: Undefined index: allow_ownvote in fivestar_custom_widget() (Zeile 449 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).
    Notice: Undefined index: description in fivestar_custom_widget() (Zeile 454 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).
    Warning: Division by zero in _fivestar_get_element_default_value() (Zeile 745 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).
    Warning: Division by zero in _fivestar_get_element_default_value() (Zeile 746 von /drupalroot/docroot/sites/all/modules/contrib/fivestar/fivestar.module).

Here are a few.
I know, why those are happening.
I am using the fivestar_custom_widget() function directly in a pane without rendering the field.
So the problem is, that indexes like 'allow_ownvote' are not set in my call of the function.
Since I do not want to set the manually everytime I use the function, I want them to fall back to the defaults.

So this is where the problem begins. There are not defaults for it.
You are checking for not set indexes already for some indexes, like here:

function fivestar_custom_widget() {
...

'#auto_submit' => isset($settings['autosubmit']) ? $settings['autosubmit'] : TRUE,
....

But you did not do this for the new indexes.

Since it is kind of annoying to check this all over the function again and again I put in a default array, which holds the defaults in one place. You could even write a function for it which alters given variables if you would want to use it somewhere else, too.

The warnings at the end could be avoided by a simple if to check if the stars value is at all set and is a positive integer.

Huge amount means briefly every time I call this fivestar_custom_widget() function, which is on every node page having 200k+ nodes and a lot of visitors, google, testing.... So I can not exactly say how much there are, but you get the picture ^^

"it seems like you fixed one description notice (after a long time), but brought like 3-4 new ones per request." - what was the one fixed? How long is a long time? What do you mean by "request"?

By "long time" no fix I meant between 2012 and 2014. Nothing happened there. I do not know, if it was reported though, but nevertheless, no stable. I know this is open source and community driven etc. I am just saying this, because you asked.
The notices which is now fixed is in `fivestar.module`:~504

Now containing this:

$form['vote']['vote']['#description'] = isset($new_element['vote']['#description']) ? $new_element['vote']['#description'] : '';

Was:

$form['vote']['vote']['#description'] = $new_element['vote']['#description'];

Request = every time I open a page with the Fivestar custom widget included. Or for the old problem, that was fixed if it was submitted.

"missing documentation" - do feel free to update the documentation. Those two options are pretty straightforward.

Even if they are, like you say "straight forward", you always say even some words to it. No one can now say, where those things take effect, what exactly they are doing and when.
I know there is no such specific rule in the Drupal Coding Conventions, but I am pretty sure, you can put this one under "document your function parameters".
Drupal itself is badly documented at most parts. If you compare it to stuff like CakePHP (which is not a CMS, I know) it is the worst and nothing more than an API. I think the community should be a role model for Drupal instead of the other way round.
I would write it, but I rather like you to do it, because you can do it faster and more accurate than me, who has to check all the code for exact functionality.

Sorry, for the late answer. Did not get an email or something. I have to check my notifications.

Regards
func0der

--- EDIT:

I forgot: I ran the updates.

func0der’s picture

No update on this one?

func0der’s picture

Okay...now you have to be kidding me.

You asked me what I mean by "long" and now you have not answered in this "direct" conversation for over 2 months?

func0der’s picture

Hello?

firesidelibrarian’s picture

A similar issue has arisen in the 'stable' version, FYI.

Will your patch work on the stable version, fundc0der?

  • whiteph committed 83df7fb on 7.x-2.x authored by func0der
    Issue #2220139 by func0der: Notices everywhere when using custom widgets
    
whiteph’s picture

Title: Notices everwhere » Notices everywhere when using custom widgets
Status: Active » Fixed

@func0der - first my apologies for not replying or handling this sooner. I had to stop my unpaid voluntary work on Fivestar to focus on my day job. I now have the time to do more work on Fivestar.

As you'll see, I've just committed your patch, and hopefully it will propagate into the dev version soon. Your extended description of the problem was very good, and I was able to read the patch in the context of custom widgets. Your use of custom widgets explains why I wasn't seeing the same proliferation of notices, I was testing with the standard UI.

I've edited the title to show the custom widget detail, so that future searches may more easily pick it up.

Thanks once again for your extended description, and your explanation of better Drupal coding practices.

Status: Fixed » Closed (fixed)

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