Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120698
MIME-Version: 1.0
References: <CAFPFaM+2eUsN9YUdUBEyr6XTe7QagyDBS8_F_LZH00ujJM8shw@mail.gmail.com>
 <72F136AA-768F-4577-80EF-22212DB50EF3@gmail.com> <CAFPFaMJEByW8Wt-ppgYHxAud4GAXTvtgC4OyrK6DfNsmO=hvLQ@mail.gmail.com>
 <7C9992E3-015E-48E6-A816-0AD012CA5CAE@gmail.com>
In-Reply-To: <7C9992E3-015E-48E6-A816-0AD012CA5CAE@gmail.com>
Date: Tue, 27 Jun 2023 14:44:09 +0100
Message-ID: <CAFPFaM+j=B0Ycf4pZP68mrY0YWCZ+AQqqFhxek75Ye5CvMf6-w@mail.gmail.com>
To: Claude Pache <claude.pache@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000001fb4f205ff1caa43"
Subject: Re: [PHP-DEV] [RFC] Deprecate remains of string evaluated code assertions
From: george.banyard@gmail.com ("G. P. B.")

--0000000000001fb4f205ff1caa43
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

I am going to group the response from the 3 emails.

On Tue, 27 Jun 2023 at 09:17, Claude Pache <claude.pache@gmail.com> wrote:

> Although the specific comment in the php.ini file talks about runtime
> switching, it is in fact irrelevant whether it is set at runtime or not.
> More concretely; I use currently the settings:
>
> * `zend.assertion=3D1` as set in the global php.ini;
> * `assert.active =3D on` or `off` in my per-directory .user.ini or .htacc=
ess
> file, depending on whether I want to enable assertions by default;
> * optionally  `ini_set('assert.active', '1')` at runtime when I want to
> enable debug mode temporarily.
>
> I have established the above settings several years ago (at the time of
> PHP 7), after having carefully read the documentation (and having been
> slightly confused by the redundancy between `assert.active` and
> `zend.assertion`). I might have end up to switch between `zend.assertion =
=3D
> 0/1` instead of switching between `assert.active =3D on/off` instead; I c=
an=E2=80=99t
> say exactly why I chose the former option instead of the latter, but I
> guess that both the comment in the `php.ini` file, and the existence of
> `assert_options(ASSERT_ACTIVE, ...)` as an alternative to
> `ini_set('assert.active', ...)`, have influenced my choice.
>
> Note also that the above settings minus `zend.assertion` was the way to d=
o
> it in PHP 5 (again it is irrelevant whether it is at runtime or not), and
> many people that used assertions in PHP 5 may have continued to use that
> way without modification, as there is currently no strong reason to chang=
e
> (only `zend.assertions=3D-1` is relevant if you are especially concerned
> about micro-optimisation).
>
> Now, of course, people will adapt to the new settings. But given the
> confusing state of the options (both `zend.assertion` and `assert.active`
> must be enabled, and I am not even speaking about `assert.exception`), yo=
u
> ought to be super-clear (in the deprecation notice, in the php.ini file, =
in
> the docs), what is the the expected state: paradoxically leave
> `assert.active` and `assert.exceptions` enabled (the default value) even
> when you want to disable assertions, and playing exclusively with
> `zend.assertion`.
>

I would argue that zend.assertions=3D-1 is more than a micro-optimisation,
but that not the point.
Yes, you would need to switch to toggle zend.assertions between 1 and 0 if
you would want to enable/disable assertions at run-time.
However, considering that code should behave the same even when assertions
are not compiled (with the -1 value) I don't really see the point of
toggling between both modes.

The expected state is to leave all the assert.* INI settings to their
default value and only use the zend.assertions INI setting.

The state of the assert() docs were in very bad shape, and I hope that my
recent changes to it has improved it and made them clearer.
However, I will make sure that the path forward is very clear be that in
the INI setting docs, the assert() docs, the assert_options() docs, the
migratrion guide, and in the deprecation message.


On Tue, 27 Jun 2023 at 09:37, Claude Pache <claude.pache@gmail.com> wrote:

> I don=E2=80=99t see the RFC listed under https://wiki.php.net/rfc#under_d=
iscussion
> .
>

Fixed.


> The RFC is imprecise in what is meant by =E2=80=9Cdeprecating=E2=80=9D. I=
 guess that a
> deprecation notice (E_DEPRECATED) will be triggered at least under the
> following conditions:
>
> * at startup when one of the assert.* setting has not the default value;
> * at runtime when `assert_options(...)` is used;
> * at runtime when `ini_set(...)` is used to set an `assert.*` option to a
> non-default value?
>
> It is unclear to me what will happen when:
>
> * `ini_set(...)` is used to set an `assert.*` option to its default value=
,
> either as a no-op or not?
>

Clarified.


> Moreover, by removing `assert.callback` (and other options), are you
> effectively removing a feature? (I don=E2=80=99t really know, I have neve=
r
> considered it even in my worst dreams.) If so, it should be noted in a
> =E2=80=9CBackward Incompatible Changes=E2=80=9D.
>

Yes, I don't really see the point of such a section as a deprecation, and
thus removal is clearly a BC Break.
I have tried to improve the wording to convey this more clearly.

On Tue, 27 Jun 2023 at 09:47, Claude Pache <claude.pache@gmail.com> wrote:

> changing the return value of `assert(...)` from `bool` (true) to `void` i=
s
> also a BC break, (and it is unclear to me what is the effective advantage
> of the break).
>

Considering that any failed assertion is going to abort execution, having
it return a value is pointless.
Moreover, one must not rely on the expression being asserted to be executed
(thus it should have no side effects) and changing it to void very clearly
conveys this meaning that one must not store the result of the assert() as
it is not a totally normal function call.

Best regards,

George P. Banyard

--0000000000001fb4f205ff1caa43--