Skip to content

Constant redefinition - #1947

Closed
dstogov wants to merge 4 commits into
php:masterfrom
dstogov:constant_redefinition
Closed

dstogov wants to merge 4 commits into
php:masterfrom
dstogov:constant_redefinition

Conversation

@dstogov

@dstogov dstogov commented Jun 16, 2016

Copy link
Copy Markdown
Member

No description provided.

@otoby

otoby commented Jun 16, 2016

Copy link
Copy Markdown

Hi Dmitry,
Your efforts for PHP are greatly appreciated.

Regarding this change, this is a huge problem for many project out there.
Consider the following

// opt.php

// index.php

// util.php

This kind of coding is common in PHP. while this shows a notice, programmers simply ignore it.
Elevating the Notice to exception is simply a disaster for many projects out there. Could you please consider a DEPRECATION message instead?

@dstogov

dstogov commented Jun 17, 2016

Copy link
Copy Markdown
Member Author

I don't see how your example is affected.
It defines a constant only once.

Thanks. Dmitry.

On Thu, Jun 16, 2016 at 1:54 PM +0300, "Ojutiku Oluwatobi" <notifications@github.commailto:notifications@github.com> wrote:

Hi Dmitry,
Your efforts for PHP are greatly appreciated.

Regarding this change, this is a huge problem for many project out there.
Consider the following

// opt.php

// index.php

// util.php

This kind of coding is common in PHP. while this shows a notice, programmers simply ignore it.
Elevating the Notice to exception is simply a disaster for many projects out there. Could you please consider a DEPRECATION message instead?

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1947#issuecomment-226452644, or mute the threadhttps://github.com/notifications/unsubscribe/ACZM0nTbqq2cOLyGFhawPrZKxYOOiexaks5qMStPgaJpZM4I3Jtc.

@laruence

Copy link
Copy Markdown
Member

this one is affected, it requires opt.php(util.php requires opt.php) twice.

you should use require_once("opt.php") anyway

@otoby

otoby commented Jun 17, 2016

Copy link
Copy Markdown

True.

What I was describing above is that folks out there assume since a constant "cannot/should not" be changed once defined. There is less/no need to bother about where it is defined and therefore, it can be imported n times.

Thanks @dstogov @laruence .

I read the internals mail on decision to consider a Deprecate message instead.

send2_opline &&
Z_TYPE(ZEND_OP1_LITERAL(send2_opline)) <= IS_STRING) {

if (collect_constants

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check inside the if looks to have been used more than once. Could this possible be pulled into its own (possible static?) function? It also might help with understanding exactly what this check is doing.

@nikic

nikic commented Jul 5, 2016

Copy link
Copy Markdown
Member

Closing this for now, as the RFC (https://wiki.php.net/rfc/constant_redefinition) has been delayed for 8.0.

@nikic nikic closed this Jul 5, 2016
@dstogov
dstogov deleted the constant_redefinition branch October 14, 2019 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants