I know you said that E_NOTICEs were less important. The fact is that if I get 100 on my screen, I'll feel like not doing anything about it. If I get one or two, I'm likely going to pay attention. So fixing them as I find them is the best way to get to the important ones... 8-)

This being said, the best here would be to have proper documentation for each function since right now we do not have any clue of what the $extra array is expected to include (although, it is clear that 'score' may not be defined since $extra is an optional parameter.)

See patch attached, although I will check this in immediately.

Thank you.
Alexis Wilke

CommentFileSizeAuthor
spam-6.x-score_notice.patch610 bytesAlexisWilke

Comments

AlexisWilke’s picture

AlexisWilke commented
Status: Needs review » Fixed

Committed:
http://drupal.org/cvs?commit=473774

gnassar’s picture

gnassar commented

You really should wait for patches to be reviewed, as a rule, even for simple ones. It costs only a little bit of patience to do so, and ensures the community is on board with changes. And especially with minor severity issues -- what's the hurry?

Considering our use of zero scores in parts of this code, it may be necessary to examine using isset() instead of !empty() here. Not a big deal for now, though.

AlexisWilke’s picture

AlexisWilke commented

From the documentation you say that the score is from 1 to 99. However, it is correct that the Baysian score can go to zero. However, the use of empty() is a replacement to what the code was doing before...

if (!$extra['score']) {
 [...]

would return true when the score is zero.

It is not clear yet whether we want to support a score of zero. Actually, if I were to completely revamp the module, I would not clamp the score to 1 to 99. Instead, I would clamp it to something like -500 to +500 so there is a limit, but that way we can take all sorts of things in account that weight in and out without losing a lot of precision as it is now.

Thank you.
Alexis

AlexisWilke’s picture

AlexisWilke commented

Btw, there is no rush, just that I don't see the point is annoying hundreds with something that little, as you say. If you want, I could open another 100 issues and never have them closed because you consider that they are so small problems that they are not worthy of being checked. Plus, if it takes you several months to check, that makes them even less worthy. It's fixed for me, if someone has a problem later, we can address that instead. 8-)

gnassar’s picture

gnassar commented

I never said they're not worthy of being checked. :) I said NOTICEs are useful, but getting rid of NOTICEs as an end unto itself is not -- NOTICEs often mean there is an error somewhere *else*, so they have to be explored more deeply. You can't just try to get rid of them; that misses the point.

In other words, if you get rid of the NOTICE the easy way, no, it's not fixed for you, because you never found the problem. :)

This issue is as good an example as any. Yes, your use of !empty($extra['score']) gives us the same functionality as the original !$extra['score']. But is that the *correct* functionality? The NOTICE shouldn't tell you, "hey, I should get rid of this notice." It should tell you, "hey, is this code correct in the first place?"

And the documentation is correct when it says the score should be from 1 to 99. That's non-dev documentation. In the core code, we sometimes seem to use a 0 score to have a special meaning. Like I've said before, we need to audit all of those cases to make sure what we're doing is consistent across the board.

But it leaves the question open as to whether we should be passing through those 0 scores in this case -- in which case isset() is the correct solution. Will we remember to check this after we check on our use of 0 scores? Well, our reminder -- the NOTICE -- is gone now, and this ticket is closed. So no, probably not. That's the problem with getting rid of NOTICEs prematurely.

AlexisWilke’s picture

AlexisWilke commented

If we really want to do things 100% correctly, we would need tests. And with our filters, tests would probably be a really good idea because it looks to me that many cases are not being checked and cause problems. This being said, that's just my feeling. It may not be true. However, as I'm testing the Duplicate module, it doesn't seem to do anything even though I posted the same comment 5 times and marked them as spam, I can continue to post that same comment over and over again... The limit is set to 2 so 5 times the spam Duplicate filter should catch the problem!

With tests, whether you remove the NOTICEs doesn't matter much because the test checks that the end result is indeed what you are supposed to get. How the code is written in between doesn't matter.

Thank you.
Alexis Wilke

gnassar’s picture

gnassar commented

Yes, we'd need tests. And we have an open ticket for them, with a patch which I'm looking at right now.

But just because we're not doing things 100% right doesn't mean we can't do them as right as possible now. Saves us a lot of trouble in the long run. And it is also what Drupal says you're supposed to do.

* Discuss all changes in separate issues. Create issues for almost all changes, even if you could commit them directly. This not only allows others to review them, but is also the only way to adhere to Drupal's guidelines for commit messages.

* Patches need to be reviewed by others and must be in RTBC state before committing them. Ideally, another project maintainer reviewed it and had no objections.
However, if patches are unnecessarily hold off because of missing reviews, then they should be committed, so development can go on. Most often, project maintainers are involved in a bunch of other projects, and they have a life, too.

(emphasis added)

So patches should exist, and they should be in RTBC (by someone else) before committing, unless it's been a reasonable while. A week is probably plenty for a simple patch. A day is not (let alone mere hours).

Status: Fixed » Closed (fixed)

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