Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow arrays with define(), to match const syntax support #951

Closed
wants to merge 1 commit into from

Conversation

hikari-no-yume
Copy link
Contributor

This fixes an oversight in PHP 5.6, where you can create constant arrays with const yet not with define(). The hope is it can go into a micro release, as it's basically just a bugfix.

@hikari-no-yume
Copy link
Contributor Author

OK, Bob Weinand pointed out I need to ensure the array contains no references or non-scalars. Will do that.

EDIT: Done, see commit below.

@@ -68,6 +68,8 @@ PHP 5.6 UPGRADE NOTES
- Added constant scalar expressions syntax.
(https://wiki.php.net/rfc/const_scalar_exprs)

- Constants can be arrays.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be a better description here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Arrays are now permitted as constant values."?

@hikari-no-yume
Copy link
Contributor Author

Made a version for master as well: #954

@hikari-no-yume
Copy link
Contributor Author

Hey @dstogov, didn't you say you'd take a look at this?

@dstogov
Copy link
Member

dstogov commented Dec 20, 2014

Hi Andrea,

Sorry, I thought you were gong to provide a patch for master.
I think it's better not to apply this in 5.6.
Even if it won't make technical problems it'll bring incompatibility
between minor 5.6.* versions.

I made a patch for master on top of yours -
https://gist.github.com/dstogov/3956efd7bbf924cfa0f8
If you don't see any problems feel free to commit it.

Thanks. Dmitry.

On Sat, Dec 20, 2014 at 6:42 AM, Andrea Faulds notifications@github.com
wrote:

Hey @dstogov https://github.com/dstogov, didn't you say you'd take a
look at this?


Reply to this email directly or view it on GitHub
#951 (comment).

@hikari-no-yume
Copy link
Contributor Author

@dstogov I already made a patch for master: #954

@hikari-no-yume
Copy link
Contributor Author

Yours seems alright, though, I'll merge it.

@hikari-no-yume
Copy link
Contributor Author

Merged here, closing this request: 0833fd4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants