Review the usage of apostrophes in error messages #5590

kocsismate · 2020-05-18T08:11:32Z

Additionally, I fixed some capitalization problems and improved some wordings.

Zend/tests/018.phpt

kocsismate · 2020-05-18T08:16:39Z

Zend/tests/invalid_parent_const_ref_leak.phpt

@@ -15,4 +15,4 @@ try {

 ?>
 --EXPECT--
-Cannot access parent:: when current class scope has no parent
+Cannot access parent when current class scope has no parent


I think the usage of :: is a bit weird, thus the removal. Can the new version still convey the message, right?

kocsismate · 2020-05-18T08:18:48Z

ext/ffi/ffi.c

@@ -3462,11 +3462,11 @@ static int zend_ffi_validate_incomplete_type(zend_ffi_type *type, zend_bool allo
 			ZEND_HASH_FOREACH_STR_KEY_PTR(FFI_G(tags), key, tag) {
 				if (ZEND_FFI_TYPE(tag->type) == type) {
 					if (type->kind == ZEND_FFI_TYPE_ENUM) {
-						zend_ffi_throw_parser_error("Incomplete 'enum %s' at line %d", ZSTR_VAL(key), FFI_G(line));
+						zend_ffi_throw_parser_error("Incomplete \"enum %s\" at line %d", ZSTR_VAL(key), FFI_G(line));


I'm not sure about these changes. Incomplete enum \"%s\", or Incomplete enum %s should be the way to go?

I'd go for Incomplete enum \"%s\".

kocsismate · 2020-05-18T08:19:49Z

ext/ffi/ffi.c

@@ -5328,7 +5328,7 @@ void zend_ffi_resolve_typedef(const char *name, size_t name_len, zend_ffi_dcl *d
 		dcl->type = type;
 		return;
 	}
-	zend_ffi_parser_error("undefined C type '%.*s' at line %d", name_len, name, FFI_G(line));
+	zend_ffi_parser_error("Undefined C type \"%.*s\" at line %d", name_len, name, FFI_G(line));


Types shouldn't be quoted, so I guess I should remove them.

kocsismate · 2020-05-18T09:55:00Z

Zend/zend_API.c

@@ -2784,7 +2784,7 @@ static int zend_is_callable_check_class(zend_string *name, zend_class_entry *sco
 	*strict_class = 0;
 	if (zend_string_equals_literal(lcname, "self")) {
 		if (!scope) {
-			if (error) *error = estrdup("cannot access self:: when no class scope is active");
+			if (error) *error = estrdup("cannot access self when no class scope is active");


In zend_ast.c we use Cannot use "self" when no class scope is active. Shouldn't these messages be consolidated into a common format? What should be the preferred format?

I'd prefer quoting "self" here (same with "parent").

Zend/tests/bug29015.phpt

nikic · 2020-06-16T13:10:20Z

Zend/zend_API.c

@@ -2784,7 +2784,7 @@ static int zend_is_callable_check_class(zend_string *name, zend_class_entry *sco
 	*strict_class = 0;
 	if (zend_string_equals_literal(lcname, "self")) {
 		if (!scope) {
-			if (error) *error = estrdup("cannot access self:: when no class scope is active");
+			if (error) *error = estrdup("cannot access self when no class scope is active");


I'd prefer quoting "self" here (same with "parent").

nikic · 2020-06-16T13:10:44Z

Zend/zend_closures.c

@@ -359,7 +359,7 @@ ZEND_METHOD(Closure, fromCallable)

 static ZEND_COLD zend_function *zend_closure_get_constructor(zend_object *object) /* {{{ */
 {
-	zend_throw_error(NULL, "Instantiation of 'Closure' is not allowed");
+	zend_throw_error(NULL, "Instantiation of Closure is not allowed");


Suggested change

zend_throw_error(NULL, "Instantiation of Closure is not allowed");

zend_throw_error(NULL, "Instantiation of class Closure is not allowed");

nikic · 2020-06-16T13:11:05Z

Zend/zend_constants.c

@@ -456,7 +456,7 @@ ZEND_API zval *zend_get_constant_ex(zend_string *cname, zend_class_entry *scope,

 	if (!c) {
 		if (!(flags & ZEND_FETCH_CLASS_SILENT)) {
-			zend_throw_error(NULL, "Undefined constant '%s'", name);
+			zend_throw_error(NULL, "Undefined constant %s", name);


I think overall I'd prefer keeping quotes here.

And what do you think about the Constant %s is deprecated error? Should I use quotes here as well? Besides, we have the Undefined constant %s::%s message where I'm unsure what to do.

I can come up with two possible logic:

We only quote global constants, and class constants remain unquoted

We treat the undefined constant error messages specially because they display non-existent (possibly bogus) constant names?

My recent commit quotes many non-existent or unfound symbols (constants, functions, methods, properties). I think I like this format, because in most cases these symbol names can be empty, or contain bogus characters, if I got it right. How do like the new format?

However, the recently changed Undefined variable $%s error message is an exception to the others, because I didn't quote it yet. Do you think it's a good idea to quote the variable name?

I think the new version went a bit too far in the other direction. The main problem with the Undefined constant %s error is that it becomes just "Undefined constant" if the name is empty, which has no indication that the name is actually empty (it would make sense as a standalone message).

I think the right rule of thumb here is to quote things that appear standalone, such as constant names, but not things that appear together with other symbols, such as undefined variables, methods etc. In particular, I don't think that Call to undefined method "Foo::()" is any clearer than Call to undefined method Foo::().

I guess another possibility, that would work for most things apart from constant names, is to check whether the symbol name contains invalid characters, and in that case print Call to undefined method Foo::{""}() and Undefined variable: ${"\xff"} and similar. Not sure if worth it though.

Thank you for the suggestions, I wasn't able to come up with so sensible rule of thumbs :) I'll try to get rid of unnecessary quotes in my next commit.

Zend/zend_execute.c

ext/ffi/tests/013.phpt

nikic · 2020-06-16T14:14:59Z

Zend/zend_API.c

@@ -331,13 +331,13 @@ ZEND_API int ZEND_FASTCALL zend_parse_arg_class(zval *arg, zend_class_entry **pc
 	*pce = zend_lookup_class(Z_STR_P(arg));
 	if (ce_base) {
 		if ((!*pce || !instanceof_function(*pce, ce_base))) {
-			zend_argument_type_error(num, "must be a class name derived from %s, '%s' given", ZSTR_VAL(ce_base->name), Z_STRVAL_P(arg));
+			zend_argument_type_error(num, "must be a class name derived from %s, \"%s\" given", ZSTR_VAL(ce_base->name), Z_STRVAL_P(arg));


The conjunction of quoted and unquoted class name here seems a bit odd. As we have a valid class here (which will use reasonable characters), I'd drop the quotes.

Yeah, makes sense! :)

nikic · 2020-06-16T14:16:18Z

ext/ffi/ffi.c

@@ -3462,11 +3462,11 @@ static int zend_ffi_validate_incomplete_type(zend_ffi_type *type, zend_bool allo
 			ZEND_HASH_FOREACH_STR_KEY_PTR(FFI_G(tags), key, tag) {
 				if (ZEND_FFI_TYPE(tag->type) == type) {
 					if (type->kind == ZEND_FFI_TYPE_ENUM) {
-						zend_ffi_throw_parser_error("Incomplete 'enum %s' at line %d", ZSTR_VAL(key), FFI_G(line));
+						zend_ffi_throw_parser_error("Incomplete \"enum %s\" at line %d", ZSTR_VAL(key), FFI_G(line));


I'd go for Incomplete enum \"%s\".

nikic

I think we should include quotes for the constant error (and generally err on the side of quotes if there can be confusion).

kocsismate · 2020-06-18T10:51:38Z

Zend/zend_API.c

@@ -1756,7 +1756,7 @@ ZEND_API int zend_startup_module_ex(zend_module_entry *module) /* {{{ */
 				if ((req_mod = zend_hash_find_ptr(&module_registry, lcname)) == NULL || !req_mod->module_started) {
 					zend_string_efree(lcname);
 					/* TODO: Check version relationship */
-					zend_error(E_CORE_WARNING, "Cannot load module '%s' because required module '%s' is not loaded", module->name, dep->name);
+					zend_error(E_CORE_WARNING, "Cannot load module \"%s\" because required module \"%s\" is not loaded", module->name, dep->name);


I'm not sure if module names should be quoted or not. The existing messages sometimes do it, sometimes not, totally inconsistently.

kocsismate · 2020-07-07T10:05:46Z

Zend/tests/assign_dim_obj_null_return.phpt

@@ -79,5 +79,5 @@ Cannot add element to the array as the next element is already occupied
 Illegal offset type
 Illegal offset type
 Cannot use a scalar value as an array
-Attempt to assign property 'foo' on bool
-Attempt to assign property 'foo' on bool
+Attempt to assign property "foo" on bool


I've just realized that for bad array containers, we use on value of type %s instead of on %s. I'm not sure which format is preferred, but personally I find the former more accurate.

kocsismate · 2020-07-07T10:09:52Z

tests/classes/array_access_002.phpt

@@ -134,11 +134,11 @@ ObjectOne::offsetGet(4th)
 int(4)
 ObjectOne::offsetGet(5th)

-Notice: Undefined index: 5th in %sarray_access_002.php on line %d
+Notice: Undefined array index "5th" in %s on line %d


is the distinction between index and offset important? If not, we should probably settle on a single name.

I think it's good to keep the distinction between "5th" and 5 (with/without quotes), but keeping the index/offset terminology is not. I would actually assume that both index and offset refer to an integer :) Maybe Undefined array key "5th", Undefined array key 5.

Yess, array key is the best choice, it didn't come to my mind before! :) I also agree with the rest, whether there are quotes or not is a very helpful thing!

nikic

From scrolling through this, I think it looks reasonable. We can adjust details later. There are some test failures that will have to be fixed first.

nikic · 2020-07-10T10:21:41Z

Zend/tests/bug39018.phpt

@@ -64,39 +64,39 @@ print "\nDone\n";
 --EXPECTF--
 Warning: String offset cast occurred in %s on line %d

-Warning: Uninitialized string offset: %s in %s on line %d
+Warning: Uninitialized string offset 430646668853805056 in %s on line %d


Probably gonna fail on 32-bit.

nikic · 2020-07-10T10:22:47Z

Zend/tests/class_name_as_scalar_error_005.phpt

@@ -7,7 +7,7 @@ $x = static::class;

 ?>
 --EXPECTF--
-Fatal error: Uncaught Error: Cannot use "static" when no class scope is active in %s:3
+Fatal error: Uncaught Error: Cannot use "static" in the global scope in %s:%d


s/in the/from the maybe?

Actually, you seem to also use from global scope in some places.

Hmm, after having a look at my changes again, I realized why I did it, and I think it was the right choice:
So in is used together with use, while from is used together with call. I think calling something from sounds good, while using something from is a bit unnatural. Another solution could be to write Cannot call "static" from the global scope, but I think calling is mainly for functions, so it's probably still not perfect.

That said, are you ok with the current wording or do you still feel that a change is needed?

No strong feelings here. From sounds more natural to me personally, but either is okay.

kocsismate · 2020-07-10T19:18:37Z

From scrolling through this, I think it looks reasonable. We can adjust details later.

Feel free to ping me should you encounter any issues! I've already noticed some room for improvement in zend_compile.c, and I'll address these in my other PR - which should be my last bigger error message related PR for PHP 8.0. :)

kocsismate force-pushed the consistent-error-messages7-apostrophes branch from d79b570 to e0f48c0 Compare May 18, 2020 08:24

kocsismate commented May 18, 2020

View reviewed changes

kocsismate force-pushed the consistent-error-messages7-apostrophes branch 3 times, most recently from d7fe04e to 77b5c19 Compare May 18, 2020 09:25

kocsismate commented May 18, 2020

View reviewed changes

kocsismate force-pushed the consistent-error-messages7-apostrophes branch 3 times, most recently from 8f453ef to db16f6f Compare May 18, 2020 11:39

kocsismate force-pushed the consistent-error-messages7-apostrophes branch from db16f6f to 68f97be Compare May 26, 2020 12:11

kocsismate force-pushed the consistent-error-messages7-apostrophes branch 4 times, most recently from 6a348b0 to 841b18a Compare June 11, 2020 07:22

nikic reviewed Jun 16, 2020

View reviewed changes

kocsismate force-pushed the consistent-error-messages7-apostrophes branch 2 times, most recently from 33ec9c9 to d03d2a7 Compare June 18, 2020 10:49

kocsismate commented Jun 18, 2020

View reviewed changes

kocsismate force-pushed the consistent-error-messages7-apostrophes branch from d03d2a7 to 1d19532 Compare June 18, 2020 12:03

kocsismate force-pushed the consistent-error-messages7-apostrophes branch 6 times, most recently from 1b3b0a7 to ea996c4 Compare July 6, 2020 07:30

This was referenced Jul 6, 2020

ext/standard warning to error promotions #5814

Closed

[RFC] Typed class constants #5815

Closed

kocsismate force-pushed the consistent-error-messages7-apostrophes branch 2 times, most recently from fad748b to 55ac989 Compare July 7, 2020 10:00

kocsismate commented Jul 7, 2020

View reviewed changes

kocsismate force-pushed the consistent-error-messages7-apostrophes branch 2 times, most recently from 4dc26dd to 2328c85 Compare July 7, 2020 17:08

nikic approved these changes Jul 10, 2020

View reviewed changes

kocsismate force-pushed the consistent-error-messages7-apostrophes branch from 2328c85 to bf868cc Compare July 10, 2020 14:58

kocsismate added 6 commits July 10, 2020 18:44

Review the usage of apostrophes in error messages

fadb8de

Adapt tests

cf0a165

A few FFI related fixes

05f6605

Address code review comments

3807cbf

Do not quote when unnecessary

3bcfaba

Handle offset and index related error messages

b3501c7

kocsismate force-pushed the consistent-error-messages7-apostrophes branch from bf868cc to e91745e Compare July 10, 2020 16:44

Address code review comments

9202feb

kocsismate force-pushed the consistent-error-messages7-apostrophes branch from e91745e to 9202feb Compare July 10, 2020 17:36

php-pulls closed this in d30cd7d Jul 10, 2020

kocsismate deleted the consistent-error-messages7-apostrophes branch July 10, 2020 19:06

owenvoke mentioned this pull request Jan 18, 2022

feat: update to use laravel/serializable-closure package amphp/parallel-functions#29

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review the usage of apostrophes in error messages #5590

Review the usage of apostrophes in error messages #5590

kocsismate commented May 18, 2020 •

edited

Loading

kocsismate May 18, 2020

kocsismate May 18, 2020

nikic Jun 16, 2020

kocsismate May 18, 2020 •

edited

Loading

kocsismate May 18, 2020 •

edited

Loading

nikic Jun 16, 2020

nikic Jun 16, 2020

nikic Jun 16, 2020

nikic Jun 16, 2020

kocsismate Jun 18, 2020

kocsismate Jun 18, 2020

nikic Jun 29, 2020

kocsismate Jun 30, 2020

nikic Jun 16, 2020

kocsismate Jun 18, 2020

nikic Jun 16, 2020

nikic left a comment

kocsismate Jun 18, 2020

kocsismate Jul 7, 2020

kocsismate Jul 7, 2020

nikic Jul 10, 2020

kocsismate Jul 10, 2020 •

edited

Loading

nikic left a comment

nikic Jul 10, 2020

nikic Jul 10, 2020

nikic Jul 10, 2020

kocsismate Jul 10, 2020 •

edited

Loading

nikic Jul 10, 2020

kocsismate commented Jul 10, 2020

	zend_throw_error(NULL, "Instantiation of Closure is not allowed");
	zend_throw_error(NULL, "Instantiation of class Closure is not allowed");

Review the usage of apostrophes in error messages #5590

Review the usage of apostrophes in error messages #5590

Conversation

kocsismate commented May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate May 18, 2020 • edited Loading

Choose a reason for hiding this comment

kocsismate May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate commented Jul 10, 2020

kocsismate commented May 18, 2020 •

edited

Loading

kocsismate May 18, 2020 •

edited

Loading

kocsismate May 18, 2020 •

edited

Loading

kocsismate Jul 10, 2020 •

edited

Loading

kocsismate Jul 10, 2020 •

edited

Loading