Everything

php

[RFC][Discussion] Make compact function reports undefined passed variables

php.internals

Gabriel Caruso

8 years ago

Hello dear internals, how are you?

I'd like to propose a new RFC to PHP's core, but as this one contains a BC
Break, let's discussed it before making anything official.

A couple of days ago, while discussing some [Coding Standards rules for
Doctrine, forbidden the `compact` function](
https://github.com/doctrine/coding-standard/pull/49), an argument caught my
attention:

> The `compact` function var does not report undefined variables.

Looking in [the `compact` documentation](https://secure.php.net/compact),
this is even emphasizes:

> Any strings that are not set will simply be skipped.

I couldn't figure out why this is done this way, but, here's what I'd like
to propose: make the `compact` function starts to report undefined passed
variables for it.

With [only 2 lines of code](
https://github.com/php/php-src/compare/master...carusogabriel:warning-unknow-compact-variable),
this is possible, but, of course, this is a BC Break.

Let me know your opinion on that, and perhaps, make it happen!

Regards,

--
Gabriel Caruso

Pieter Hordijk

8 years ago

----- Original Message -----

> From: "Gabriel Caruso" <carusogabriel34@gmail.com>
> To: "internals" <internals@lists.php.net>
> Sent: Monday, April 2, 2018 11:17:43 AM
> Subject: [PHP-DEV] [RFC][Discussion] Make compact function reports undefined passed variables

> Hello dear internals, how are you?
>
> I'd like to propose a new RFC to PHP's core, but as this one contains a BC
> Break, let's discussed it before making anything official.
>
> A couple of days ago, while discussing some [Coding Standards rules for
> Doctrine, forbidden the `compact` function](
> https://github.com/doctrine/coding-standard/pull/49), an argument caught my
> attention:
>
>> The `compact` function var does not report undefined variables.
>
> Looking in [the `compact` documentation](https://secure.php.net/compact),
> this is even emphasizes:
>
>> Any strings that are not set will simply be skipped.
>
> I couldn't figure out why this is done this way, but, here's what I'd like
> to propose: make the `compact` function starts to report undefined passed
> variables for it.
>
> With [only 2 lines of code](
> https://github.com/php/php-src/compare/master...carusogabriel:warning-unknow-compact-variable),
> this is possible, but, of course, this is a BC Break.
>
> Let me know your opinion on that, and perhaps, make it happen!
>
> Regards,
> --
> Gabriel Caruso

Hey Gabriel,

What I am missing here is the reason why we should break well defined behavior of existing functions.

How is the BC break for all existing code justified?

Pieter

Gabriel Caruso

8 years ago

>
> Hey Gabriel,
>
> What I am missing here is the reason why we should break well defined
> behavior of existing functions.
>
> How is the BC break for all existing code justified?
>
> Pieter
>

Hi Pieter.

> What I am missing here is the reason why we should break well defined

behavior of existing functions.

This is something I'd like to discuss about `compact`. I'd like to know why
we skip undefined variables instead of report them. `compact` is use in a
large scale by the MVC community, by passing variables in the controller
and using them in the view.

> How is the BC break for all existing code justified?

I'd like to start report this undefined variables. Would be one more way to
prevent code-smell. IMHO this belongs to the core, not to the userland.


Thanks,

--
Gabriel Caruso

Pieter Hordijk

8 years ago

> Hi Pieter.

>> What I am missing here is the reason why we should break well defined behavior
> > of existing functions.

> Hi Pieter.

>> What I am missing here is the reason why we should break well defined behavior
> > of existing functions.

> This is something I'd like to discuss about `compact`. I'd like to know why we
> skip undefined variables instead of report them. `compact` is use in a large
> scale by the MVC community, by passing variables in the controller and using
> them in the view.

> > How is the BC break for all existing code justified?

> I'd like to start report this undefined variables. Would be one more way to
> prevent code-smell. IMHO this belongs to the core, not to the userland.

> Thanks,

> --
> Gabriel Caruso

Regardless of the why it is like that the fact of the matter is that it is and imho
you would need a really good reason to break existing code.

I still don't don't see your reason for it. Nor do I see how it is a code smell atm.

Ivan Enderlin @ Hoa

8 years ago

Hello,

Yes, a thousand yes. Silently ignoring undefined variables is a source
of multiple bugs. Emiting a warning is a small BC break comparing to the
benefits.

Regards.

On 02.04.18 11:17, Gabriel Caruso wrote:

Andrew Faulds

8 years ago

And the determined can prefix with an @ if they want similar to old
behaviour :)

Ivan Enderlin wrote:

> Hello,
>
> Yes, a thousand yes. Silently ignoring undefined variables is a source
> of multiple bugs. Emiting a warning is a small BC break comparing to the
> benefits.
>
> Regards.
>

--
Andrea Faulds
https://ajf.me/

Björn Larsson

8 years ago

And by that one could argue that the BC break is negligible :)
Anyway, I think this is a clear improvement!

Regards

--
//Björn Larsson


Den 2018-04-09 kl. 23:54, skrev Andrea Faulds: