Remove the use of the @ operator

Thanks for this very useful piece of information. I committed the fixes.

another

notice: Undefined index: vbo_display_result in views_bulk_operations\views_bulk_operations.module on line 541.

ok, as I see this notice was fixed in the last commit, but I must tell PHP notices should not be fixed with the "@" operator..

I believe the first notice was already fixed with this change

and the second notice should be fixed like

I agree; the @ operator should not be used to hide the attempt to access an undefined index.

Thanks for your advice. However, using '@' is my intent here: if the index is undefined, then the value of the expression is NULL and the code handles that situation correctly. Actually, I think that the PHP language designers must have had such a usage in mind when they added the '@' operator. Instead of the long-hand:

one can write to the exact same effect:

since NULL evaluates to FALSE in boolean expressions.

The PHP documentation states that (see http://www.php.net/manual/en/language.operators.errorcontrol.php)

PHP supports one error control operator: the at sign (@). When prepended to an expression in PHP, any error messages that might be generated by that expression will be ignored.

This means that the code you wrote will generate an error, but the error message will not be show on screen, or in the PHP log; the PHP interpreter will be forced to continue the execution like nothing happened.
It doesn't avoid the error happens, and the only way to not make it happen is to use code like if (isset($var) && $var).

A runtime trigger is processed, yes, but I don't think the internals are significant. It certainly isn't a performance problem.

I support using @ in trivial cases for notice suppression
(Note notice - it's not a warning and certainly not an error)

So that's short, and clear.
If $in[x] didn't have a value, I totally want to get a null into the corresponding $out[y]. That's my code.

You will triple the code and severely damage readability by avoiding notices the long way.
Using @, you just end up with "Huh? Oh OK, that works".

I am not convinced, because if you use @ to hide a simple notice then you (or other contributors) will start hiding real errors with it. I am really worried about this approach. It is worse than displaying the notice. I do not really understand why the php language has such operator.
Notices may be indicative of bugs. Please do not hide them, fix them correctly with assigning a default value or using the isset() function.

I agree; to hide a bug is not the way to resolve it.

There are some cases where the operator is useful; i.e., it can be used when you call db_drop_index(), and you are not sure the index exists. In that case, as there isn't a way to first check if the database index exists, it is better to use @db_drop_index() to not show an useless error message (you already know the function could fail).

I explained my argument as to why the PHP language has such an operator. This is not a bug, this is a feature. Again, I am not hiding anything. Please understand the difference.

I will use a mix of isset() and '@' depending on the situation. I will try my best not to abuse it. Maybe instead of banning '@', we could all agree to where it can be used safely and engage other contributors in the discussion. That's certainly better than engaging on an '@'-crusade.

I don't think there are errors in that code, yet without an @ it will create a few notices. This is deliberate and conscious.
There is not a bug to be fixed there, and I'm not 'hiding' anything.
The notice is there to make the programmer think. I've done my thinking, and can say "yes, I know what I'm doing. Thanks for pointing it out to me but no, it's not a problem".
I can carry on.
If you dont understand how to use @, by all means don't use it.
If you don't understand how to use regular expressions, don't use them.
... but allow programmers that do to appropriately use the tools at their disposal.

Thanks dman for joining in.

I don't mind rules and coding conventions, but they have to make sense, and we must be ready to re-evaluate them in light of new arguments for or against. Thanks Pasqualle and Kiam for caring about the quality of the contrib modules. This discussion is the sign of a very healthy community, and Drupal certainly has one of the healthiest open source communities I've seen. I for one benefited by learning how to turn on PHP notices, and I will fix them in my modules as they turn up.

I agree on the use of the @ operator with functions that are known to throw some notices; the example I wrote on the previous comment (which was referring to function call like @db_drop_index()) should make clear I was not talking of that.
I was referring to code like if (@$test) {} which could hide bugs present in the code. The bug could be, i.e., that the if-statement has been misplaced before the code that initializes the variable.

@dman: actually there is a problem with that code. You are using the wrong functions for the task.
http://php.net/parse_url

This function is not meant to validate the given URL.

you can use: http://api.drupal.org/api/function/valid_url/6

I guess the loadXML function also only returns notice (warning or error) when there is a problem, like the XML is not valid. But there is a function for XML validation also..

If you can show me a single example in Drupal core where @ is used to hide the undefined index notice, then I will gladly change the issue status back to fixed, otherwise this is just a bad code style. And I really do care about that..

DOMDocument::loadXML() will generate a warning only when an empty string is passed like argument (http://php.net/manual/en/domdocument.loadxml.php).

Pasqualle and Kiam,

This argument is turning silly and I will waste no further time on this issue.

The fact that Drupal core uses or doesn't use the '@' operator is irrelevant. There's no need for all Drupal core and the thousands of contrib modules to be completely homogeneous in terms of coding style. Indeed, it is impossible to achieve that.

If you're so adamant to enhance the quality of Drupal, how about writing some unit tests or fixing the zillions of real bugs in core and in contrib modules. Forcing me to remove the '@' operator is probably not the most useful way to spend your time, especially that I don't intend to do it.

I don't think that somebody is forcing anybody else to do anything; it has been pointed out that the use of the @ operator is not correct in such cases.

I would then make you note the sentence you're so adamant to enhance the quality of Drupal doesn't make any sense, as adamant means refusing to be persuaded.

My top-of-the-head examples were only to illustrate that there is a valid reason for the construct to exist at all in PHP - which Pasqualle doesn't seem to agree with.
Drupal core may not use @ much, if at all. Instead it uses set_error_handler - which mutes ALL notices globally. Which could be regarded as being equally evil - if you think that all informational notices == BUG!

Drupal does implicitly acknowlege the use of @ as valid - see http://api.drupal.org/api/function/drupal_error_handler/6 so it's not like there's anything, even in the style guidelines to prohibit it. Dunno where you got that idea.
So it's just an opinion.

As far as style goes, I'd rather have clear, readable array assignments in my code - like #7 - than huge if-then bits in the middle of it, when I know that a null return is legal. PHP is nice with its soft typing and all, and is why I can code stuff up quick.
And that's why it's code style. Style is a matter of taste. I prefer readability and maintainability.

FWIW, i on a weekly basis contribute patches to all sorts of modules just to get PHP-strict warnings to go away. Almost always with a real empty() or isset(), or explicit variable declarations up higher. Only when it would damage the code with no advantage do I occasionally use @.

So I basically agree with the clean code campaign - I'm not hiding the problem! But I think it's draconic to call deliberate use of a valid part of the language a "bug".

I'd suggest that rather than criticize a maintainers accurate and valid fix for a small problem, suggestions on removing strict notices may easily accepted when supplied with a patch that does it your preferred way.
like
I
do
often

Re: adamant: yup sorry, bad usage of language, thanks for looking it up. I meant determined. I hope that makes sense now ;-)

http://drupal.org/node/327884
@dman: as I see all 3 commenters said that using @ is not good..
and a bonus: #183374-5: E_ALL bug - notice: Undefined offset: 1

The notice was already fixed like the way I prefer, but it was changed in the next commit..

I also work on notices, but before I submit a patch I rather tell the maintainer how to display the notice, as it would be already fixed if the maintainer knew that there is a bug..

I would agree that

is an inappropriate use.
There are plenty of ways to demonsrtate a bad use of any given language feature at all. That is one.
One person using it wrong does not mean that nobody should use it ever.

I still haven't seen evidence of the claim from two the folk that claim that notice suppression is more inefficient than passing the error all the way up the chain and logging it. I call that claim bogus.

It's true that Drupal uses set_error_handler(), but it does that so it can show the notices on screen only if it has been set to do so (as Drupal has a setting to decide if the notices must appear on screen, on log, or both).

I, and Pasqualle are not criticizing any people; we are talking of the wrong use of a PHP operator without to give any judgement to the person who wrote the code, differently from somebody else who express a personal judgement on us.
I find not necessary, and contrary to the spirit of Drupal.org, the reply given from kratib; as Drupal.org suggests to not create a new projects when there is already a project for the same purpose, it's obvious that people report anything they think wrong in the code of the existing project. To reply go to do something else means to express an opinion about people that is not required; as we didn't express any opinion about the author of the module (we could have said the same words referring to him), he should do the same.

The use of the @ operator is not thought to be used when you are calling the wrong function (like Pasqualle showed about parse_url()), nor to be used like shortcut to write less code, nor to be used so that the code is not stopped when there is a bug in it.
A variable can be NULL because it has not been initialized; therefore, the bug could be to use a variable before it gets any value (maybe because the if-statement in which the initializing code is in is wrong).

The fact that Drupal core uses or doesn't use the '@' operator is relevant; it shows that there is a group of developers who know the existence of the @ operator, but decided to not use it. At this point, one person should think why didn't they use such operator?; if it was so easy to use it to access a variable that could be NULL, why didn't they use it? Imagine how much code they could have saved; would not have it been fantastic? A free way to write less code, and to show that Drupal does the same things of another CMS with less code.

@dman: in your comment #21, I read

I would agree that

is an inappropriate use.

The subject of the subordinate clause is not visible.

(this could be done slightly shorter and more obscurely with ternary assignments, however ternary is certainly not really to be regarded as recommended best-practice)

I'm just saying I find the first example to be better code in terms of clarity and maintainabilty. Also size and efficiency, but even those are not my priorities. And it's much easier to write!
I do not think that that code is an "error".
In my opinion. You can disagree and say the second is actually more clear, maintainable, small, efficient and easier, but that can be your opinion.

The second example has the advantage of proving that you can get the same effect with more code by deliberately avoiding using a part of the language that was provided to help you.
It is equally (no more or less) accurate or correct as the first in that it performs the same result with the same effect. *
Is there anything else about the second example is better than the first?

This issue is looking to me like the argument that you should write all logic checks in the form
if (CONST == $check)
instead of
if ($check == CONST)
There is a valid reason for believing this.
The claimed reason is that you are less likely to inadvertently make the syntax error of if ($check = CONST)
This is true. And the world is saved. Anyone who writes code by following that rule must obviously be producing better code.
</sarcasm>

My (extremely roundabout) point is that making life difficult for yourself in order to avoid making silly mistakes is more likely to have the opposite effect in these cases.
At a glance, which of A or B is more likely to be bug-free?
I call example B above "making life difficult for yourself" in order to avoid the totally unfounded fear of ending up with a NULL value exactly where you wanted a NULL value or of writing code that is just plain wrong in other ways.

Me, I'm comfortable with softly-typed languages, and am happy that NULL prints out as an empty string and evaluates to 0 and FALSE. That's a feature of this language I'm allowed to take advantage of. This is WHY I don't enjoy strictly typed languages. But that's choice.

Leave me my choice to use this language feature, and I'll leave you with your choice not to.

.dan.

_{* this is a lie}

I am not sure the first example would be an example of maintainabilty, if it would hide a bug.

Suppose that the code is something like

Suppose that check_value() never returns 1, but it returns 10 and the code should have been really written like if (check_value() == 10) {; in a case like that, using the @ operator would simply hide a bug present in the code which would not be easily found because there are no notices.

The comparison you make about using if (CONST == $check) if ($check == CONST) is wrong. CONST == $check</em>, and <code>$check == CONST both returns TRUE when the variable value is equal to the constant value; $out = array( 'description' => $in['desc']), and $out = @array( 'description' => $in['desc']) doesn't have the same effect, as the first would throw a notice (while the second would not).

I still haven't seen evidence of the claim from two the folk that claim that notice suppression is more inefficient than passing the error all the way up the chain and logging it. I call that claim bogus.

The difference is not in term of efficiency; the difference is that you will not be able to notice a warning about an error in the code because you are suppressing the error message.

So you find the second version more maintainable than the first? If someone came after me and had to update the code, they would do a better job if the code looked like the second version?
That, as I said, can be your opinion.

Talk abut hiding bugs? I hid one already in plain sight in example B. How long does it take to find it?

My point about CONST == $check is not at all about the result, or even the error handling.
It's about a (seriously proposed) code convention that is designed just to make sure you avoid a typo.
- occasionally someone would type $check = CONST by mistake. Accidental assingment where comparison was wanted. This mistake would be impossible if they were typing CONST = $check.

I say that in this case - and in the case of replacing @ with an extra 10 lines of code - the prevention is worse than the danger.

The guys Pasqualle linked to in http://drupal.org/node/381838#comment-1297540 were talking about efficiency as the reason to avoid @. ... I didn't believe them.

@ is just shorthand for a try(){ do something } catch() { do nothing } construct.
A warning deliberately caught is not an error. That's what error triggers are all about. Bubble up the issue until someone catches it. I caught it and handled the consequences.
Modern languages throw exceptions all the time. Knowing about where exceptions may occur, and handling them appropriately is common in stricter languages.
Error handling is part of code! Catching a trigger means the event is handled. And no longer an error.

The example code should look like this:

It is not an equivalent, I purposely changed NULL to empty string, so you have a default value for the $out array, and you don't have to check it next time, when you use any index from it. (or you could fix the $in array before using it) .

Using @ is like sweeping the problems under the carpet, it looks good from outside but the mess is still there..

If by maintainability you mean that it's easier to find out a bug, then it is.

You can say that is my opinion, but it's an opinion shared by many other people.