[RFC]: Change Default mysqli Error Mode

3 years ago by Larry Garfield — view source

unread

Hi Internals,

Following my earlier announcement in https://wiki.php.net/rfc/improve_mysqli
I decided to submit a dedicated RFC for the first proposal.

This RFC proposes to change the default mysqli error reporting mode to
exception mode. This change follows the same change in PDO. The RFC can be
found here https://wiki.php.net/rfc/mysqli_default_errmode

Kind Regards,
Kamil Tekiela

Consistency FTW. +1

--Larry Garfield

3 years ago by BohwaZ — view source

unread

We should do the same with SQLite3 extension.

3 years ago by bjorn.x.larsson@telia.com — view source

unread

Den 2021-01-21 kl. 00:04, skrev Kamil Tekiela:

Hi Internals,

Following my earlier announcement in https://wiki.php.net/rfc/improve_mysqli
I decided to submit a dedicated RFC for the first proposal.

This RFC proposes to change the default mysqli error reporting mode to
exception mode. This change follows the same change in PDO. The RFC can be
found here https://wiki.php.net/rfc/mysqli_default_errmode

Kind Regards,
Kamil Tekiela

If I understand this correctly, does it imply that a lot of
legacy code that works today, will not work any longer and
needs to change? Like:
if ("mysqlierror") {
"error handling"
}

If so I think that like in other RFC's an investigation should
be done on how much code that is affected to assess BC impact.

Btw, I liked your proposal on how to improve prepared statements
in mysqli a lot!

Regards //Björn Larsson

3 years ago by Kamil Tekiela — view source

unread

On Thu, 21 Jan 2021 at 21:51, Björn Larsson bjorn.x.larsson@telia.com
wrote:

If I understand this correctly, does it imply that a lot of
legacy code that works today, will not work any longer and
needs to change? Like:
if ("mysqlierror") {
"error handling"
}

If so I think that like in other RFC's an investigation should
be done on how much code that is affected to assess BC impact.

The code will still work as it did before, but if there is an error then an
exception will be thrown. This means that by default the custom error
handlers will become redundant. Generally, this should not be a problem,
but I can imagine that some projects heavily rely upon the manual error
checking.

Among the popular PHP projects that I know of, only CodeIgniter and
WordPress still use mysqli.
CI does offer PDO as an option but due to legacy reasons, many people still
chose mysqli. However, they already use the exception mode.
https://github.com/codeigniter4/CodeIgniter4/blob/d5673f4376dc981196607bc159368bee9990033c/system/Database/MySQLi/Connection.php#L94
WordPress ... the black sheep of PHP ... uses mysqli with a fallback to
mysql_* API. They use manual error handling, which would be replaced by
normal exceptions unless they silence it. They should modernize their
codebase at some point as they can't cling on to the past forever. For the
moment if they want to continue using manual error checking then they need
to explicitly disable mysqli errors without relying on the default setting.
That is only 1 additional line of code. This would be the same as what I
have done in PHP internal test cases. I don't see any problems there.

In fact, I don't really see this as a major breaking change. If any project
really needs the mysqli errors silenced then they can just set the error
mode back to OFF. The change is aimed at new users, not existing projects.
Silenced error reporting was a source of confusion for plenty of beginners
who were unaware of the setting.

Kind Regards,
Kamil Tekiela

3 years ago by Christian Schneider — view source

unread

Am 21.01.2021 um 23:46 schrieb Kamil Tekiela tekiela246@gmail.com:

In fact, I don't really see this as a major breaking change. If any project
really needs the mysqli errors silenced then they can just set the error
mode back to OFF. The change is aimed at new users, not existing projects.
Silenced error reporting was a source of confusion for plenty of beginners
who were unaware of the setting.

As this definitely is a BC (applications stop running) I think we should go through a phase were it throws a warning instead of aborting, i.e. changing it to
mysqli_report(MYSQLI_REPORT_ERROR);
first and change it to throw an exception after a grace period.

This would be in line with most other newly introduced exceptions for the default configuration.
Plus this would most certainly help people who host other people's websites who want to upgrade to PHP 8.x.

PS: IMHO The same should be (retroactively) done with round() and friends!

Chris

3 years ago by G. P. B. — view source

unread

On Mon, 25 Jan 2021 at 15:41, Christian Schneider cschneid@cschneid.com
wrote:

Am 21.01.2021 um 23:46 schrieb Kamil Tekiela tekiela246@gmail.com:

In fact, I don't really see this as a major breaking change. If any
project
really needs the mysqli errors silenced then they can just set the error
mode back to OFF. The change is aimed at new users, not existing
projects.
Silenced error reporting was a source of confusion for plenty of
beginners
who were unaware of the setting.

As this definitely is a BC (applications stop running) I think we should
go through a phase were it throws a warning instead of aborting, i.e.
changing it to
mysqli_report(MYSQLI_REPORT_ERROR);
first and change it to throw an exception after a grace period.

This would be in line with most other newly introduced exceptions for the
default configuration.
Plus this would most certainly help people who host other people's
websites who want to upgrade to PHP 8.x.

PS: IMHO The same should be (retroactively) done with round() and friends!

Chris

--

To unsubscribe, visit: https://www.php.net/unsub.php

The BC break is totally minimal as it's a one line of code that needs to be
added (and for all intent and purposes should already be done).

Moreover, I'd rather we get rid of the warning modes all together as they
make the least sense to me.
Either you're handling the failures explicitly anyway and you use the
silent mode, or you don't and want it to throw with the exception mode.
The warning mode is literally the worst of both worlds, it doesn't fail on
failure so you need to check for failure manually,
and you need to suppress the warnings because otherwise you get false
negatives.

Best regards,

George P. Banyard

3 years ago by Benjamin Morel — view source

unread

Moreover, I'd rather we get rid of the warning modes all together as they
make the least sense to me.
Either you're handling the failures explicitly anyway and you use the
silent mode, or you don't and want it to throw with the exception mode.
The warning mode is literally the worst of both worlds, it doesn't fail on
failure so you need to check for failure manually,
and you need to suppress the warnings because otherwise you get false
negatives.

Best regards,

George P. Banyard

I'd just like to say amen to that.

Please move as much code as possible to throwing exceptions, and get rid of
warnings. Most of the time when a warning is thrown in PHP, what you
actually want is to deal with it, or stop execution.

Benjamin

3 years ago by Christian Schneider — view source

unread

Am 25.01.2021 um 16:59 schrieb G. P. B. george.banyard@gmail.com:

The BC break is totally minimal as it's a one line of code that needs to be added (and for all intent and purposes should already be done).

That does not change the fact that it is a BC break which should be treated accordingly.

You are basically advocating people should add mysqli_report(MYSQLI_REPORT_OFF) to their setup which will totally defeat the purpose of the change.
Or (probably more what you have in mind) that they should add mysqli_report(MYSQLI_REPORT_ERROR) now (to prepare for the change) and switch to MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT later once they know they are ready.
And this is exactly what I think should be done by the PHP default so people don't have to go through this manually.

Moreover, I'd rather we get rid of the warning modes all together as they make the least sense to me.
Either you're handling the failures explicitly anyway and you use the silent mode, or you don't and want it to throw with the exception mode.

This is where you are missing a key component: I want to go through a phase where I will notice parts where manual error handling (or exception handling after the change) is missing without having my application abort. This is what warnings are good for.

To some of us this is a very valuable step when upgrading.

Chris

3 years ago by G. P. B. — view source

unread

On Mon, 25 Jan 2021 at 16:25, Christian Schneider cschneid@cschneid.com
wrote:

Am 25.01.2021 um 16:59 schrieb G. P. B. george.banyard@gmail.com:

The BC break is totally minimal as it's a one line of code that needs to
be added (and for all intent and purposes should already be done).

That does not change the fact that it is a BC break which should be
treated accordingly.

You are basically advocating people should add
mysqli_report(MYSQLI_REPORT_OFF) to their setup which will totally defeat
the purpose of the change.
Or (probably more what you have in mind) that they should add
mysqli_report(MYSQLI_REPORT_ERROR) now (to prepare for the change) and
switch to MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT later once they know
they are ready.
And this is exactly what I think should be done by the PHP default so
people don't have to go through this manually.

Moreover, I'd rather we get rid of the warning modes all together as
they make the least sense to me.
Either you're handling the failures explicitly anyway and you use the
silent mode, or you don't and want it to throw with the exception mode.

This is where you are missing a key component: I want to go through a
phase where I will notice parts where manual error handling (or exception
handling after the change) is missing without having my application abort.
This is what warnings are good for.

To some of us this is a very valuable step when upgrading.

Chris

I've never said there is no BC break, and frankly this change (and other
similar ones) should probably have been done in PHP 8 at the same time with
PDO.

And yes I'm basically advocating for people to add
mysqli_report(MYSQLI_REPORT_ERROR) (or even mysqli_report(MYSQLI_REPORT_OFF)
if they fancy) to their setup, I'm even advocating for them to do it right
now such that they can catch anything ahead of time.
Moreover, this proposal does not prevent you from using the warning mode
instead of jumping to exceptions directly, the key point is that you need
to specify it, and I personally believe that having the warning mode as the
default mode is the worst choice for the reasons I mentioned previously.
The removal of the warning modes is completely orthogonal to this proposal,
and I agree it is a very useful tool for transitioning to the exception
mode, but this transition can already be done in PHP 5, 7 and 8.

The grace period for completing this transition is from whenever you want
to whenever, if ever, the warning mode gets deprecated and removed.

On the note that a one line code change is not a good enough justification
for saner defaults, I don't know what to say.
At worst you need to find all usages of a connection start and add the line
to downgrade the error reporting (even to silence if too many warnings are
emitted).
I don't see what testing needs to be done, you're reverting to the previous
state which supposedly is already working.

Best regards,

George P. Banyard

3 years ago by Christian Schneider — view source

unread

Am 25.01.2021 um 18:22 schrieb G. P. B. george.banyard@gmail.com:

And yes I'm basically advocating for people to add mysqli_report(MYSQLI_REPORT_ERROR) (or even mysqli_report(MYSQLI_REPORT_OFF) if they fancy) to their setup, I'm even advocating for them to do it right now such that they can catch anything ahead of time.

Defaults matter. Most people will not do this step and their code will fail on upgrade.

Moreover, this proposal does not prevent you from using the warning mode instead of jumping to exceptions directly, the key point is that you need to specify it, and I personally believe that having the warning mode as the default mode is the worst choice for the reasons I mentioned previously.

Agree to disagree.

If I have the choice to make the smoothen the transition centrally with PHP defaults or rely on each user to do it manually by proactively setting warning modes I prefer the defaults way.

Remember: I am not advocating staying in the warning mode, I'm just seeing it as a useful intermediate step for legacy stuff we want to fix.

Chris

3 years ago by drealecs@gmail.com — view source

unread

On Mon, Jan 25, 2021 at 7:33 PM Christian Schneider cschneid@cschneid.com
wrote:

Am 25.01.2021 um 18:22 schrieb G. P. B. george.banyard@gmail.com:

And yes I'm basically advocating for people to add
mysqli_report(MYSQLI_REPORT_ERROR) (or even
mysqli_report(MYSQLI_REPORT_OFF) if they fancy) to their setup, I'm even
advocating for them to do it right now such that they can catch anything
ahead of time.

Defaults matter. Most people will not do this step and their code will
fail on upgrade.

Moreover, this proposal does not prevent you from using the warning mode
instead of jumping to exceptions directly, the key point is that you need
to specify it, and I personally believe that having the warning mode as the
default mode is the worst choice for the reasons I mentioned previously.

Agree to disagree.

If I have the choice to make the smoothen the transition centrally with
PHP defaults or rely on each user to do it manually by proactively setting
warning modes I prefer the defaults way.

Remember: I am not advocating staying in the warning mode, I'm just seeing
it as a useful intermediate step for legacy stuff we want to fix.

Chris

Hi Chris,

I think PHP is far from targeting a zero cost on version upgrade for
projects.
There will be other parts that will require testing, whatever you do.
Trying to optimize this to reduce the 1 line quick fix that could be easily
read in the documentation at https://www.php.net/manual/en/migration81.php
sounds to me like we're trying too hard to reduce this cost, ignoring
others. Other technical costs would be to not switch earlier to new
exception mode as long as the default will cover the cases.

I am thinking about this with a legacy system in mind that is long due on
exception handling for the database layer. I know that having exceptions by
default would spark an interest in developers' minds to do this change
sooner rather than later because they will feel the default is the
recommended way. Due to bandwidth allocation, they might add the line
there, but a task will be created to fix it in the next few months, maybe a
year.

There will be a cost of upgrading. New scenarios and tests that might not
even exist currently, negative flow scenarios.

Regards,
Alex

3 years ago by Christian Schneider — view source

unread

Am 26.01.2021 um 07:57 schrieb Alexandru Pătrănescu drealecs@gmail.com:

There will be other parts that will require testing, whatever you do. Trying to optimize this to reduce the 1 line quick fix that could be easily read in the documentation at https://www.php.net/manual/en/migration81.php https://www.php.net/manual/en/migration81.php sounds to me like we're trying too hard to reduce this cost, ignoring others.

The migration guides are already getting quite long, just have a look at https://www.php.net/manual/de/migration80.incompatible.php https://www.php.net/manual/de/migration80.incompatible.php
"Just one more line" can add up and that's why I want to be able to proactively fix my code, not fixing it at the time of migration.

If the error mode would be changed to warnings instead of exceptions first then every developer interested in best practices could and would already adapt their code. But they could do it gradually without the disruption of hard fails looming over them.

I just think giving developers a warning period (per default, not opt-in) is the right thing to do for BC breaks. [ Did I mention round()? ;-) ]

I start to sound like a broken record, I'll leave it at that,

Chris

3 years ago by Kamil Tekiela — view source

unread

I would like to say that I completely agree with George. The BC is minimal.
We are not introducing any new functionality or removing any either. We are
just changing a default setting that was always there. The only code that
would be affected is the one that is currently relying on the default
setting. Most of the mysqli code should already have the setting
enabled/disabled explicitly. If your current code has mysqli error
reporting disabled then nothing will change for you. If your current code
has mysqli error reporting enabled then nothing will change for you. It
will only change in the code that doesn't have it enabled/disabled yet.
Such a project would need to decide whether to enable it or disable it.
This can be done even now, and it should be done now, regardless of whether
this RFC is accepted or not.

Or (probably more what you have in mind) that they should add
mysqli_report(MYSQLI_REPORT_ERROR) now (to prepare for the change) and
switch to MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT later once they know
they are ready.

IMHO the warning mode is useless as it doesn't stop the code execution
automatically but still throws an error. You either enable error reporting
in the form of exceptions, or you keep it silenced. I believe changing the
default mode to warning mode makes no sense.
Bear in mind that you can do it right now. There's no need for you to wait
for PHP 8.1 to see the change. You can decide on the error reporting mode
right now. However, I fail to see how mysqli_report(MYSQLI_REPORT_ERROR)
could help in migrating the code. As long as your code doesn't have any
errors, the error reporting mode will make no difference.

This is where you are missing a key component: I want to go through a
phase where I will notice parts where manual error handling (or exception
handling after the change) is missing without having my application abort.
This is what warnings are good for.

Enabling warning mode will not help you in finding places where you are
missing manual error handling. That is the whole point. Manual error
handling means that you have to do it manually. When you enable warnings or
exceptions then manual error handling is already useless. If you think that
your code is missing some manual error handling then enabling
warning/exception mode will not reveal to you where these places are. You
have to go line by line through your code and everwhere you see a mysqli
method/function call you have to add manual error handling.

I want to make things straight. Changing the default error mode is not
forcing anyone to use exception mode out of the sudden. You can still
continue to use whichever mode you are using right now. The change is aimed
at people who start their new projects with PHP 8.1 and want to learn
mysqli.

I don't see what testing needs to be done, you're reverting to the
previous state which supposedly is already working.

I also do not know what kind of additional testing would be required. The
functionality stays the same. A project code should remain the same unless
its maintainers decide to refactor it. All that is needed is to make sure
that the project has the correct mode enabled and does not rely on the
default setting.

3 years ago by Nikita Popov — view source

unread

On Tue, Jan 26, 2021 at 8:42 AM Christian Schneider cschneid@cschneid.com
wrote:

Am 26.01.2021 um 07:57 schrieb Alexandru Pătrănescu drealecs@gmail.com:

There will be other parts that will require testing, whatever you do.
Trying to optimize this to reduce the 1 line quick fix that could be easily
read in the documentation at https://www.php.net/manual/en/migration81.php
https://www.php.net/manual/en/migration81.php sounds to me like we're
trying too hard to reduce this cost, ignoring others.

The migration guides are already getting quite long, just have a look at
https://www.php.net/manual/de/migration80.incompatible.php <
https://www.php.net/manual/de/migration80.incompatible.php>;
"Just one more line" can add up and that's why I want to be able to
proactively fix my code, not fixing it at the time of migration.

If the error mode would be changed to warnings instead of exceptions first
then every developer interested in best practices could and would already
adapt their code. But they could do it gradually without the disruption of
hard fails looming over them.

I just think giving developers a warning period (per default, not opt-in)
is the right thing to do for BC breaks. [ Did I mention round()? ;-) ]

I start to sound like a broken record, I'll leave it at that,

Chris

I think if you wanted to introduce an additional diagnostic step, the way
to go about it would be to issue a deprecation warning when creating a
connection without mysqli_report() having been explicitly called
beforehand. As others have said, the warning mode is pretty much entirely
useless, and what you really want (presumably) is to know about the places
where you need to explicitly call mysqli_report() to preserve the old
behavior (or explicitly switch to the new one).

Regards,
Nikita

3 years ago by Christian Schneider — view source

unread

Am 27.01.2021 um 14:34 schrieb Nikita Popov nikita.ppv@gmail.com:

I think if you wanted to introduce an additional diagnostic step, the way to go about it would be to issue a deprecation warning when creating a connection without mysqli_report() having been explicitly called beforehand.

Wouldn't this mean everybody would add mysqli_report() to their code?
And wouldn't this render the default of mysqli_report() entirely useless?
Or is your idea that at a later stage you then want to create a warning for manual mysqli_report() calls?

As others have said, the warning mode is pretty much entirely useless, and what you really want (presumably) is to know about the places where you need to explicitly call mysqli_report() to preserve the old behavior (or explicitly switch to the new one).

No, I want to know if I have any code which needs fixing so I can see how I want to change those places e.g. a duplicate key from INSERT to REPLACE or whatever the correct fix is. Without the code aborting.

Yes, individual developers can switch on mysqli_report(MYSQLI_REPORT_ERROR) now to get this behavior but I still think this makes more sense as a default. Otherwise the temptation is big to just add a global mysqli_report(MYSQLI_REPORT_OFF) everywhere just in case.

Chris

3 years ago by Nikita Popov — view source

unread

On Wed, Jan 27, 2021 at 3:35 PM Christian Schneider cschneid@cschneid.com
wrote:

Am 27.01.2021 um 14:34 schrieb Nikita Popov nikita.ppv@gmail.com:

I think if you wanted to introduce an additional diagnostic step, the
way to go about it would be to issue a deprecation warning when creating a
connection without mysqli_report() having been explicitly called beforehand.

Wouldn't this mean everybody would add mysqli_report() to their code?
And wouldn't this render the default of mysqli_report() entirely useless?
Or is your idea that at a later stage you then want to create a warning
for manual mysqli_report() calls?

Yes, that would render a call to mysqli_report() required until the version
where the default was switched, and the requirement went away. A call to
mysqli_report() is already required if you use exception-based error
handling. The deprecation would force those people who use manual error
handling by default to make an explicit choice.

As others have said, the warning mode is pretty much entirely useless,
and what you really want (presumably) is to know about the places where you
need to explicitly call mysqli_report() to preserve the old behavior (or
explicitly switch to the new one).

No, I want to know if I have any code which needs fixing so I can see how
I want to change those places e.g. a duplicate key from INSERT to REPLACE
or whatever the correct fix is. Without the code aborting.

Okay, I think I now get what you want. My line of thinking was that people
who use manual error handling likely just want to keep doing that, and
should opt-in to it once the default changes. You apparently want them to
switch to use exceptions, in which case the warning mode might indeed be
useful to identify cases where "expected" errors occur.

However, I don't think this has much to do with the proposal at hand. If
someone wants to migrate their code to use exceptions, that's of course
great, but they could do that any time, regardless of what the default is.
They could have done that ten years ago, and they could do it in ten years.

This proposal is only about switching the default, which is why I mentioned
the possibility of diagnosing cases where it may be observable. That said,
I don't think doing so would actually be particularly useful in practice.
And it wouldn't even address your particular concern. As such, I think the
RFC should stick with the original proposal of simply flipping the default
directly.

Regards,
Nikita

3 years ago by Kamil Tekiela — view source

unread

Wouldn't this mean everybody would add mysqli_report() to their code?

That is the idea. That is what everyone should already have in their
codebase.

And wouldn't this render the default of mysqli_report() entirely useless?

I'm sorry, but I don't follow the train of thought? Why would it make that
useless? You must set the correct error reporting mode anyway and that
function helps in doing so.

Or is your idea that at a later stage you then want to create a warning

for manual mysqli_report() calls?

What are manual mysqli_report() calls? You either have that line in your
code or you don't. The idea that Nikita suggested is to meet your concerns.
The only way to figure out if your code needs an adjustment is to have a
warning emitted from any mysqli connect methods if you have not called
mysqli_report before. It would inform users that they are missing the
setting.

No, I want to know if I have any code which needs fixing so I can see how

I want to change those places e.g. a duplicate key from INSERT to REPLACE
or whatever the correct fix is. Without the code aborting.

You can do that right now. This proposal will not make it easier or more
difficult for you to do that. Just paste mysqli_report(MYSQLI_REPORT_ERROR)
before mysqli_connect() and PHP will generate a warning whenever your SQL
throws an error.

Otherwise the temptation is big to just add a global

mysqli_report(MYSQLI_REPORT_OFF) everywhere just in case.

I don't get what's wrong with that. This is literally what I said in the
RFC you can do if you don't want mysqli to report any errors. Whoever
doesn't want to see errors can use this line to silence them.
I would even make a counterpoint. If we set MYSQLI_REPORT_ERROR as the
default then users will definitely silence errors and forget. The warning
mode is so useless and annoying that people will prefer to silence it
seeing no benefit to it. If we want this change to be useful then we should
make SQL errors report as exceptions by default.

Regarding Nikita's proposal: The compromise is reasonable, but this will
not be well-received by the community. Additional warnings will only make
things more confusing without providing outstanding benefits. If we want to
force people to set the error reporting mode that they want, then it would
be better to set a sensible default value and give a clear explanation in
the migration guide how to set it back to the previous value. Of course, I
don't hate Nikita's proposal and I might say I even like it a little bit,
but only in connection with exception mode as the default value. This way
users will not be surprised by an exception if it happens that they forget
to silence. But I would prefer that everyone embraced exceptions.

3 years ago by bjorn.x.larsson@telia.com — view source

unread

Den 2021-01-27 kl. 16:07, skrev Kamil Tekiela:

Wouldn't this mean everybody would add mysqli_report() to their code?

That is the idea. That is what everyone should already have in their
codebase.

And wouldn't this render the default of mysqli_report() entirely useless?

I'm sorry, but I don't follow the train of thought? Why would it make that
useless? You must set the correct error reporting mode anyway and that
function helps in doing so.

Or is your idea that at a later stage you then want to create a warning

for manual mysqli_report() calls?

What are manual mysqli_report() calls? You either have that line in your
code or you don't. The idea that Nikita suggested is to meet your concerns.
The only way to figure out if your code needs an adjustment is to have a
warning emitted from any mysqli connect methods if you have not called
mysqli_report before. It would inform users that they are missing the
setting.

No, I want to know if I have any code which needs fixing so I can see how

I want to change those places e.g. a duplicate key from INSERT to REPLACE
or whatever the correct fix is. Without the code aborting.

You can do that right now. This proposal will not make it easier or more
difficult for you to do that. Just paste mysqli_report(MYSQLI_REPORT_ERROR)
before mysqli_connect() and PHP will generate a warning whenever your SQL
throws an error.

Otherwise the temptation is big to just add a global

mysqli_report(MYSQLI_REPORT_OFF) everywhere just in case.

I don't get what's wrong with that. This is literally what I said in the
RFC you can do if you don't want mysqli to report any errors. Whoever
doesn't want to see errors can use this line to silence them.
I would even make a counterpoint. If we set MYSQLI_REPORT_ERROR as the
default then users will definitely silence errors and forget. The warning
mode is so useless and annoying that people will prefer to silence it
seeing no benefit to it. If we want this change to be useful then we should
make SQL errors report as exceptions by default.

Regarding Nikita's proposal: The compromise is reasonable, but this will
not be well-received by the community. Additional warnings will only make
things more confusing without providing outstanding benefits. If we want to
force people to set the error reporting mode that they want, then it would
be better to set a sensible default value and give a clear explanation in
the migration guide how to set it back to the previous value. Of course, I
don't hate Nikita's proposal and I might say I even like it a little bit,
but only in connection with exception mode as the default value. This way
users will not be surprised by an exception if it happens that they forget
to silence. But I would prefer that everyone embraced exceptions.

I'm missing a thing here when we talk about "your code". I mean on our
site we have third party libraries. They can typically have a release
schedule on a couple of releases each year. So all code is not up to
us to change. So how does this proposal adress that scenario when it's
not up to us to do the necessary changes? Is it e.g. a blocker to
upgrade to PHP 8.1 with exceptions vs warnings until the library is updated?

As a side note I'm at the moment migrating our test site to PHP 8 and
the two most prominent hickups are located in a third party library:
"Undefined array key" and "Attempt to read property "value" on null"
which are warnings. Not sure how it would have been with these as
exceptions, e.g. would the test site stop running and thereby stop
testing of other parts.

And as I have said earlier, I do like exceptions! It's the migration
aspect I'm wondering about...

r//Björn L

3 years ago by bjorn.x.larsson@telia.com — view source

unread

Den 2021-01-27 kl. 17:13, skrev Björn Larsson:

Den 2021-01-27 kl. 16:07, skrev Kamil Tekiela:

Wouldn't this mean everybody would add mysqli_report() to their code?

That is the idea. That is what everyone should already have in their
codebase.

   And wouldn't this render the default of mysqli_report() entirely
useless?

I'm sorry, but I don't follow the train of thought? Why would it make
that
useless? You must set the correct error reporting mode anyway and that
function helps in doing so.

   Or is your idea that at a later stage you then want to create a
warning

for manual mysqli_report() calls?

What are manual mysqli_report() calls? You either have that line in your
code or you don't. The idea that Nikita suggested is to meet your
concerns.
The only way to figure out if your code needs an adjustment is to have a
warning emitted from any mysqli connect methods if you have not called
mysqli_report before. It would inform users that they are missing the
setting.

   No, I want to know if I have any code which needs fixing so I can
see how

I want to change those places e.g. a duplicate key from INSERT to
REPLACE
or whatever the correct fix is. Without the code aborting.

You can do that right now. This proposal will not make it easier or more
difficult for you to do that. Just paste
mysqli_report(MYSQLI_REPORT_ERROR)
before mysqli_connect() and PHP will generate a warning whenever your
SQL
throws an error.

   Otherwise the temptation is big to just add a global

mysqli_report(MYSQLI_REPORT_OFF) everywhere just in case.

I don't get what's wrong with that. This is literally what I said in the
RFC you can do if you don't want mysqli to report any errors. Whoever
doesn't want to see errors can use this line to silence them.
I would even make a counterpoint. If we set MYSQLI_REPORT_ERROR as the
default then users will definitely silence errors and forget. The
warning
mode is so useless and annoying that people will prefer to silence it
seeing no benefit to it. If we want this change to be useful then we
should
make SQL errors report as exceptions by default.

Regarding Nikita's proposal: The compromise is reasonable, but this will
not be well-received by the community. Additional warnings will only
make
things more confusing without providing outstanding benefits. If we
want to
force people to set the error reporting mode that they want, then it
would
be better to set a sensible default value and give a clear
explanation in
the migration guide how to set it back to the previous value. Of
course, I
don't hate Nikita's proposal and I might say I even like it a little
bit,
but only in connection with exception mode as the default value. This
way
users will not be surprised by an exception if it happens that they
forget
to silence. But I would prefer that everyone embraced exceptions.

I'm missing a thing here when we talk about "your code". I mean on our
site
we have third party libraries. They can typically have a release
schedule on
a couple of releases each year. So all code is not up to us to change.
So how
does this proposal adress that scenario when it's not up to us to do the
necessary changes? Is it e.g. a blocker to upgrade to PHP 8.1 with
exceptions vs warnings until the library is updated?

As a side note I'm at the moment migrating our test site to PHP 8 and
the two most prominent hickups are located in a third party library:
"Undefined array key" and "Attempt to read property "value" on null"
which are warnings. Not sure how it would have been with these as
exceptions, e.g. would the test site stop running and thereby stop
testing of other parts.

And as I have said earlier, I do like exceptions! It's the migration
aspect I'm wondering about...

r//Björn L

PS And sorry for the lousy formatting in earlier response.

3 years ago by Kamil Tekiela — view source

unread

I'm missing a thing here when we talk about "your code". I mean on our
site we have third party libraries. They can typically have a release
schedule on a couple of releases each year. So all code is not up to
us to change. So how does this proposal adress that scenario when it's
not up to us to do the necessary changes? Is it e.g. a blocker to
upgrade to PHP 8.1 with exceptions vs warnings until the library is
updated?

As a side note I'm at the moment migrating our test site to PHP 8 and
the two most prominent hickups are located in a third party library:
"Undefined array key" and "Attempt to read property "value" on null"
which are warnings. Not sure how it would have been with these as
exceptions, e.g. would the test site stop running and thereby stop
testing of other parts.

And as I have said earlier, I do like exceptions! It's the migration
aspect I'm wondering about...

r//Björn L

This is a valid concern. I have thought about it for a while and I think
that we should handle this the same way we did with PDO error mode change.
We don't do anything special.

I really don't see a way that we could make a smoother upgrading
experience. For users that avail of old libraries the onus is on them to
check the library's compatibility with the new PHP version. Ideally, the
change should have been made in PHP 8.0 where it would be more justifiable
to change a default setting.

The good thing about mysqli error reporting compared to PDO is that the
setting is global. If the library you are using is abandoned or you can't
wait for the library to be updated then you can set the error reporting
level in your own code. The only problem comes when you want to mix both
modes in the same code, but that is a huge code smell anyway. This means
that this change is not a blocker for the upgrade.

I think the proposal is good to be implemented in PHP 8.1 as it is now. But
if anyone thinks otherwise or has any ideas on how to improve I would
gladly take it into consideration. If there is any way that we could make
the migration smoother then it would be great.

Kamil

3 years ago by bjorn.x.larsson@telia.com — view source

unread

Den 2021-01-28 kl. 01:31, skrev Kamil Tekiela:

I'm missing a thing here when we talk about "your code". I mean on
our
site we have third party libraries. They can typically have a release
schedule on a couple of releases each year. So all code is not up to
us to change. So how does this proposal adress that scenario when it's
not up to us to do the necessary changes? Is it e.g. a blocker to
upgrade to PHP 8.1 with exceptions vs warnings until the library
is updated?

As a side note I'm at the moment migrating our test site to PHP 8 and
the two most prominent hickups are located in a third party library:
"Undefined array key" and "Attempt to read property "value" on null"
which are warnings. Not sure how it would have been with these as
exceptions, e.g. would the test site stop running and thereby stop
testing of other parts.

And as I have said earlier, I do like exceptions! It's the migration
aspect I'm wondering about...

r//Björn L
This is a valid concern. I have thought about it for a while and I
think that we should handle this the same way we did with PDO error
mode change. We don't do anything special.

I really don't see a way that we could make a smoother upgrading
experience. For users that avail of old libraries the onus is on them
to check the library's compatibility with the new PHP version.
Ideally, the change should have been made in PHP 8.0 where it would be
more justifiable to change a default setting.

The good thing about mysqli error reporting compared to PDO is that
the setting is global. If the library you are using is abandoned or
you can't wait for the library to be updated then you can set the
error reporting level in your own code. The only problem comes when
you want to mix both modes in the same code, but that is a huge code
smell anyway. This means that this change is not a blocker for the
upgrade.

I think the proposal is good to be implemented in PHP 8.1 as it is
now. But if anyone thinks otherwise or has any ideas on how to improve
I would gladly take it into consideration. If there is any way that we
could make the migration smoother then it would be great.

Kamil

Yes, this proposal would probably have been close to a no-brainer in
PHP 8.0.

I think the RFC would benefit on expanding on some of the key issues
that has been discussed, e.g. the clarification above about migration
aspects.

/r//Björn L/

3 years ago by Kamil Tekiela — view source

unread

Hi All,

I have updated the RFC at https://wiki.php.net/rfc/mysqli_default_errmode
and provided FAQ with the aim to explain your concerns as best as I could.
I have also added a section that explains all mysqli error reporting modes.

Best regards,
Kamil

3 years ago by Kamil Tekiela — view source

unread

Hi All,

If there are no more comments I would like to start voting on this RFC
soon. If you would like me to change something or if you want to discuss it
further please let me know.

Kind Regards,
Kamil

Hi All,

I have updated the RFC at https://wiki.php.net/rfc/mysqli_default_errmode
and provided FAQ with the aim to explain your concerns as best as I could.
I have also added a section that explains all mysqli error reporting modes.

Best regards,
Kamil

3 years ago by Nikita Popov — view source

unread

Hi Internals,

Following my earlier announcement in
https://wiki.php.net/rfc/improve_mysqli
I decided to submit a dedicated RFC for the first proposal.

This RFC proposes to change the default mysqli error reporting mode to
exception mode. This change follows the same change in PDO. The RFC can be
found here https://wiki.php.net/rfc/mysqli_default_errmode

Kind Regards,
Kamil Tekiela

Fully support this proposal. Throwing exceptions is the right default, it
matches what PDO does, and restoring the old behavior is one line of code.

Regards,
Nikita

3 years ago by Sebastian Bergmann — view source

unread

Am 25.01.2021 um 11:03 schrieb Nikita Popov:

Fully support this proposal. Throwing exceptions is the right default, it
matches what PDO does, and restoring the old behavior is one line of code.

I second that emotion.

3 years ago by johannes@schlueters.de — view source

unread

Am 25.01.2021 um 11:03 schrieb Nikita Popov:

Fully support this proposal. Throwing exceptions is the right
default, it
matches what PDO does, and restoring the old behavior is one line
of code.

I second that emotion.

The issue is, as said, that this only happens in an error situation.
Thus if users only test "does my site still work after update?" (what
many do ...) won't notice this, till something fails, which would have
been caught nice beforehand.

I agree that the proposed behavior is better, but we have to mind
migration paths.

johannes

3 years ago by Kamil Tekiela — view source

unread

The issue is, as said, that this only happens in an error situation.
Thus if users only test "does my site still work after update?" (what
many do ...) won't notice this, till something fails, which would have
been caught nice beforehand.

I don't see why this would be such an issue. If the code fails for any
reason then an error is produced. The error might be surprising, but it is
not something that would cause problems. In situations where the code
depends on the silenced errors to work properly then the developers would
know this and they would test for it during the upgrade.
This change fixes more problems than it creates. I believe it is still
worthy to put it in 8.1. Every PHP release contains backwards-incompatible
changes. In comparison to other BC changes PHP has put in minor releases
this one has a very easy fix. You don't have to scour the code looking for
places to fix.
PHP can't improve if we have to think of people who upgrade without reading
the release notes. We also shouldn't be held back by outdated tutorials. In
fact, this will help people who still use such tutorials. This isn't a
major change that would affect everyone on the planet. This will only
affect people who still use mysqli and didn't set the error reporting mode
before upgrading.
I understand your worries but I don't think this is a good enough reason to
not go ahead with this change.

Kind regards,
Kamil

3 years ago by Rowan Tommins — view source

unread

I don't see why this would be such an issue. If the code fails for any
reason then an error is produced. The error might be surprising, but it is
not something that would cause problems.

Just addressing this argument, not the proposal itself, a function
throwing an exception that previously failed silently absolutely could
cause problems.

Consider this code:

function get_product($product_id) {
$sql = "Select * From products Where product_id = '" .
$dbWrapper->escape($product_id) . "'";
return $dbWrapper->runQuery($sql);
}

If product_id is actually an integer column, this function is
technically broken: given a non-integer input, it will produce an error
in the database. However, with lax error handling, the calling code will
simply treat this as the product not existing, and handle it gracefully.

If $dbWrapper->runQuery suddenly starts throwing exceptions, functions
that were inadvertently relying on this behaviour will start failing,
possibly with an uncaught exception propagating to the user.

The exception is certainly pointing out a real bug, and the function can
easily be fixed once the problem is spotted, but the change has a real
negative impact on the application.

To be clear, I am not saying I agree or disagree with this particular
proposal. I note that the user could in this example update their DB
wrapper to set the error mode explicitly, or to catch exceptions. I just
wanted to show that "it only happens on error" doesn't mean the impact
is zero.

Regards,

--
Rowan Tommins
[IMSoP]

3 years ago by Kamil Tekiela — view source

unread

If product_id is actually an integer column, this function is
technically broken: given a non-integer input, it will produce an error
in the database.

I'm sorry, but I do not understand why would that code produce an error.
The value is properly escaped and formatted so there should be no error at
all. Is this based on some SQL setting? That SQL looks correct to me and I
do not see any syntax errors.
Also, silenced or not, if an error occurs on MySQL side then PHP has to
check for it one way or another or it will break with some other horrendous
error.

3 years ago by Rowan Tommins — view source

unread

If product_id is actually an integer column, this function is
technically broken: given a non-integer input, it will produce an
error
in the database.
I'm sorry, but I do not understand why would that code produce an
error. The value is properly escaped and formatted so there should be
no error at all. Is this based on some SQL setting? That SQL looks
correct to me and I do not see any syntax errors.

That's precisely why it's the kind of code that goes unfixed for years,
but it is broken: if $product_id is the string 'hello world', then this
line...

$sql = "Select * From products Where product_id = '" .
$dbWrapper->escape($product_id) . "'";

...produces this SQL:

Select * From products Where product_id = 'hello world'

If product_id is a column of type int, then the database will raise an
error about incompatible types.

If the PHP database wrapper just swallows this error and returns false,
then somewhere else in the code, you can write this:

$product = get_product($_GET['product_id']);
if ( ! $product ) {
display_error_message('Sorry, we could not find that product.');
}

So although the code is wrong, the user always gets a reasonable error
message. Now replace the database implementation with one that throws
exceptions, and that message will never display; if you're lucky,
there's a default exception handler set; if not, the user will get a
blank white page.

Regards,

--
Rowan Tommins
[IMSoP]

3 years ago by Kamil Tekiela — view source

unread

Select * From products Where product_id = 'hello world'

If product_id is a column of type int, then the database will raise an
error about incompatible types.

No, this query would produce a warning. I am unaware of an SQL mode that
would turn this warning into an error. Maybe I am wrong and there is a way
to convert this into an error, but that would be pointless.
I believe you have chosen the wrong example. That query, even when executed
with PDO, doesn't throw an exception.

The point of this RFC is that mysqli should treat SQL errors the same way
that PDO does. By default, all errors should be reported. An error in SQL
is the same as an exception in PHP code; it means that something went
terribly wrong. I have been using PDO for a few years now and I never had a
need to silence its error reporting. SQL doesn't spit out exceptions
randomly.

Examples, that would truly be effected are something like this:

$stmt = $mysqli->query('INSERT INTO tableA (uniqueColumn) VALUE("repeated
value")');
if (1062 === $mysqli->errno) {
echo "Can't add this value because it already exists!";
}

If the developer didn't silence the errors then the user will see a generic
error instead of the specific one.

3 years ago by Kamil Tekiela — view source

unread

The code shown here https://www.w3schools.com/php/func_mysqli_connect.asp is
relatively correct at the moment. Maybe not nice, but it handles the error.
That is the second hit on google (after php.net manual) when searching for
"php tutorial mysqli_conect") Most other books, tutorials, ... teach the
same.

w3schools is hardly something that should be regarded as a reputable source
of information and should not impact the direction PHP is heading in.
This problem comes mostly from the fact that the developers who wrote these
tutorials were unaware that there is automatic error reporting available.
PHP manual didn't explain this either. This RFC proposes to change that by
making it the default so that people can't miss it.
The reason why I really want this default setting to change is that I was
lied to by books and tutorials when I was learning PHP. I wish I knew about
automatic error reporting instead of struggling with invisible SQL errors.
I still hold a grudge against the tutorials teaching me manual error
checking or the use of real_escape_string. I do not want new developers in
the era of PHP 8 to suffer the same pain I did.

Spend a day looking at stackoverflow and you see a different world. Also
many users just want to use a PHP app and don't care about PHP.

Yes, but once again the problem is the old tutorials that teach it
incorrectly. Most of them still are vulnerable to SQL injection. We can't
change the tutorials but we can change the recommended setting in PHP.

In 8.1 add deprecation notices to mysqli_connect & friends AND to access
to the connection_error (if I remember right that goes via the __get hook,
thus we can add a deprecation note to a "property" access)
The later is a good place as many people use the @ operator on the
connect, but not on the error check.
In a later version we change it.
That gives a chance to reach users. We will still miss many, but for a
notable amount of people there is a chance.

I believe Nikita has proposed it already. I am not very happy about it as
this would mostly go unnoticed. People who have mysqli error reporting
silenced often have every other error reporting silenced too including
deprecation notices. Adding such deprecation notice would just create
another confusing PHP error that developers would have to fix. I am sorry
to say this about my fellow developers but many of the ones that you are
targeting with this proposal who wouldn't read an upgrade guide before
upgrading, are also the ones who would ignore this deprecation warning in
their error logs. "Hey, the code still works, I don't know what this
deprecation notice is talking about!"
For these reasons, I am unwilling to go through a deprecation phase. I
believe it would be just a waste of time. But if this is the only way that
this change will land in PHP then I would also accept it as a very
unsatisfying compromise.

3 years ago by johannes@schlueters.de — view source

unread

The code shown here
https://www.w3schools.com/php/func_mysqli_connect.asp is
relatively correct at the moment. Maybe not nice, but it handles the
error.
That is the second hit on google (after php.net manual) when searching
for
"php tutorial mysqli_conect") Most other books, tutorials, ... teach
the
same.

w3schools is hardly something that should be regarded as a reputable
source
of information

That is correct. I wouldn't advice following that.

and should not impact the direction PHP is heading in.

That I don't agree with. The things covered in tutorials and books is what people use to learn. What people learn is what they use for some time in their path. We have a responsibility towards our users.

This problem comes mostly from the fact that the developers who wrote
these
tutorials were unaware that there is automatic error reporting
available.

For many that is true. In some cases tutorial writers are happy about the fact that they don't have to teach exceptions early.

Error handling in PHP is a mess, but I consider the path towards more exceptions good. But has to be done with care.

PHP manual didn't explain this either. This RFC proposes to change that by
making it the default so that people can't miss it.
The reason why I really want this default setting to change is that I
was
lied to by books and tutorials when I was learning PHP. I wish I knew
about
automatic error reporting instead of struggling with invisible SQL
errors.
I still hold a grudge against the tutorials teaching me manual error
checking or the use of real_escape_string. I do not want new developers
in
the era of PHP 8 to suffer the same pain I did.

These tutorials are a pain and there is a large group of people creating worst quality content and hoping to benefit from SEO stuff and then earning some money from ads. Others focus on content and don't get attention. That is a big problem we're having.

I disagree however that throwing out errors the readers don't understand helps. How many will say "oh the tutorial is broken" and how many will say "my PHP is broken"? Back in my days, I remember, I was more often looking for failures by me or my system configuration than blaming the tutorial.

[...]

In 8.1 add deprecation notices to mysqli_connect & friends AND to
access
to the connection_error (if I remember right that goes via the __get
hook,
thus we can add a deprecation note to a "property" access)
The later is a good place as many people use the @ operator on the
connect, but not on the error check.
In a later version we change it.
That gives a chance to reach users. We will still miss many, but for
a
notable amount of people there is a chance.

I believe Nikita has proposed it already. I am not very happy about it
as
this would mostly go unnoticed. People who have mysqli error reporting
silenced often have every other error reporting silenced too including
deprecation notices. Adding such deprecation notice would just create
another confusing PHP error that developers would have to fix.

The difference is: With a deprecation warning we control the error message better.

I am sorry
to say this about my fellow developers but many of the ones that you are
targeting with this proposal who wouldn't read an upgrade guide before
upgrading, are also the ones who would ignore this deprecation warning in
their error logs. "Hey, the code still works, I don't know what this
deprecation notice is talking about!"
For these reasons, I am unwilling to go through a deprecation phase. I
believe it would be just a waste of time. But if this is the only way
that
this change will land in PHP then I would also accept it as a very
unsatisfying compromise.

I don't know why there is a specific time pressure. That behavior is now almost twenty years old.

And we'll, about visibility: Tutorial writers hopefully don't ignore it and can catch up. Also deprecation warnings have a chance to be reported to the authors etc. Yes, it won't reach everybody, but we're talking here about a change affecting huge numbers of systems. If we help just a few hundred thousands of those to make a smoother transition it's already a good thing.

And yes, there are more people reading deprecstion notices than changelogs. Outside the enthusiasts only few do. Partially since most entries are irrelevant, partially since there are so many changelogs one could read all day, partially since people don't even know what version they are running or that the host is being upgraded by an admin.

People look at it, if they notice a problem. But they first need a way to even notice. Silently ignoring half the code (the existing error handling) goes unnoticed.

Last thing: I truly think it is great that your trying to push this forward! We always need people coming up here and fixing the painpoints they had! It is easy for people here to get reluctant to change over time and becoming used to the way things are. I only consider the change too rushed.

johannes

P.S. Most people here know it, I assume, but as disclaimer: I am a member of the MySQL engineering team at Oracle, where these days, I'm not working on anything regarding Client APIs. All opinions stated are my own.

3 years ago by Kamil Tekiela — view source

unread

Your opinion is highly appreciated, Johannes. I feel that we will have to
implement this change as you are proposing.
I don't think I am rushing this change, because I believe is already late.
It should have been fixed in PHP 7 or PHP 8, but because very little people
use mysqli this extension hasn't seen much love. I know we can't remove
mysqli completely so I am trying to improve the situation a little bit. The
reason why I think the time is now is because of changes to PDO default
error reporting mode and the move from Warnings to Errors in PHP 8. Moving
to mysqli exceptions makes sense now. Other warnings in mysqli extension
have already been turned to exceptions.
I don't think the deprecation notice will reach as many people as we hope,
but at least it will give us peace of mind.
I am sorry if I sound a little bit defensive about my ideas, but I know
from experience that many developers are allergic to any kind of change and
I have to use a lot of convincing to achieve anything.
I'll close the voting tomorrow and then I will think on how to adapt the
RFC to deploy deprecation notice first.

Thanks.
Kamil

3 years ago by Rowan Tommins — view source

unread

Select * From products Where product_id = 'hello world'

If product_id is a column of type int, then the database will raise an
error about incompatible types.
No, this query would produce a warning. I am unaware of an SQL mode
that would turn this warning into an error.

Right, I forgot we were talking about MySQL, which has the same "ignore
and carry on" tradition as PHP; in other DBMSes, that would be an error:

PostgreSQL:

ERROR: invalid input syntax for type integer: "hello world"
LINE 1: Select * From products Where product_id = 'hello world';

Microsoft SQL Server:

Msg 245 Level 16 State 1 Line 1
Conversion failed when converting the varchar value 'hello world' to
data type int.

However, the main point of the example was to show that an error could
go unnoticed, and actually have no immediate negative effects, such that
replacing it with an exception makes things worse.

Examples, that would truly be effected are something like this:

$stmt = $mysqli->query('INSERT INTO tableA (uniqueColumn)
VALUE("repeated value")');
if (1062 === $mysqli->errno) {
echo "Can't add this value because it already exists!";
}

If the developer didn't silence the errors then the user will see a
generic error instead of the specific one.

Precisely, and this is what people are concerned about: if someone is
writing code from scratch, exception-by-default is a good thing; but if
they have existing error handling, by-passing that handling by throwing
an exception is not.

Regards,

--
Rowan Tommins
[IMSoP]

3 years ago by johannes@schlueters.de — view source

unread

The issue is, as said, that this only happens in an error situation.
Thus if users only test "does my site still work after update?" (what
many do ...) won't notice this, till something fails, which would
have
been caught nice beforehand.

I don't see why this would be such an issue. If the code fails for any
reason then an error is produced. The error might be surprising, but it
is
not something that would cause problems. In situations where the code
depends on the silenced errors
to work properly

The code shown here https://www.w3schools.com/php/func_mysqli_connect.asp is relatively correct at the moment. Maybe not nice, but it handles the error. That is the second hit on google (after php.net manual) when searching for "php tutorial mysqli_conect") Most other books, tutorials, ... teach the same.

then the developers
would
know this and they would test for it during the upgrade.

That is true for the ones posting here and talking at conferences.
Spend a day looking at stackoverflow and you see a different world. Also many users just want to use a PHP app and don't care about PHP. In many cases some agency (which often have no idea about software development, but mostly care about the UI design and reaching a goal quickly) created an app and the users just want to run it and got an server with thestest version. This isn't nice, but it is a reality.

This change fixes more problems than it creates. I believe it is still
worthy to put it in 8.1.

It is true that more consistent error reporting via Exceptions would be better and solve many problems. These are the reminders of the "OO wars" we fought around the time of the PHP 4 - PHP 5 transition and I think it is great that many people are working on cleaning this up!

Every PHP release contains
backwards-incompatible
changes.

There is a balance we're always fighting about. There are cases where BC breaks are done and hopefully most of them explode loud. For instance of we add a new keyword. This emits and error quite quickly, a simple lint check is enough to find it. Just including the file will fail.

Here we are talking about a case where the break will only occur in an errorous situation. Which users will only spot when reading the upgrading guide carefully. In an area 99% of beginner tutorials and books touch.

In comparison to other BC changes PHP has put in minor
releases
this one has a very easy fix. You don't have to scour the code looking
for places to fix.

The fix is easy. Getting awareness is hard.

PHP can't improve if we have to think of people who upgrade without
reading
the release notes. We also shouldn't be held back by outdated
tutorials. In
fact, this will help people who still use such tutorials. This isn't a
major change that would affect everyone on the planet. This will only
affect people who still use mysqli and didn't set the error reporting
mode
before upgrading.
I understand your worries but I don't think this is a good enough
reason to
not go ahead with this change.

I am not about stagnating and not doing anything. As said: I think the idea is good. I am about thinking about the process and timing.

What about this approach:

In 8.1 add deprecation notices to mysqli_connect & friends AND to access to the connection_error (if I remember right that goes via the __get hook, thus we can add a deprecation note to a "property" access)

The later is a good place as many people use the @ operator on the connect, but not on the error check.

In a later version we change it.

That gives a chance to reach users. We will still miss many, but for a notable amount of people there is a chance.

johannes

3 years ago by Benjamin Eberlei — view source

unread

The issue is, as said, that this only happens in an error situation.
Thus if users only test "does my site still work after update?" (what
many do ...) won't notice this, till something fails, which would have
been caught nice beforehand.

I don't see why this would be such an issue. If the code fails for any
reason then an error is produced. The error might be surprising, but it is
not something that would cause problems.

I think you are very wrong about this, there could be perfectly valid code
doing:

if (!$conn->query("select 1") && $conn->errno == 2006) { // mysql gone away
$conn = new mysqli(....);
}

With your proposed change the query would throw an exception instead,
potentially breaking follow up code that is required to run.

This is the kind of code you have that only gets triggered every 100000
times and that is very hard to write a real test for. If the test here uses
a mock, then you wont catch the behavior change.

This change fixes more problems than it creates. I believe it is still
worthy to put it in 8.1. Every PHP release contains backwards-incompatible
changes. In comparison to other BC changes PHP has put in minor releases
this one has a very easy fix. You don't have to scour the code looking for
places to fix.

There are projects out there that do a mysqli_connect on every script.
You underestimate the impact of this break.

PHP can't improve if we have to think of people who upgrade without reading

the release notes. We also shouldn't be held back by outdated tutorials. In
fact, this will help people who still use such tutorials. This isn't a
major change that would affect everyone on the planet. This will only
affect people who still use mysqli and didn't set the error reporting mode
before upgrading.

I am sorry, but to me that feels a bit like you want to take the easy way
out..

As a counterexample, Nikita made a very good RFC for 8.1 / 9.0 deprecating
and changing the nullable to internal functions behavior:
https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg

This RFC goes above and beyond to provide a migration path, deprecation
messages and such.

Yes it has a higher potential break impact, but i don't expect this change
to wait until 9.0 either.

I would wish your RFC would throw a deprecation message in 8.1 that you
cannot make a query without setting the reporting level before by calling
mysqil_report() explicitly. Then in 8.2 we could change the behavior.

I understand your worries but I don't think this is a good enough reason to
not go ahead with this change.

Kind regards,
Kamil