Skip to content

Disallow invoking GMP constructor #10158

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

Closed
wants to merge 1 commit into from
Closed

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Dec 22, 2022

Fixes GH-10155

Note: I targeted master because I wasn't sure whether this could be considered a BC break.

@Girgias
Copy link
Member

Girgias commented Dec 23, 2022

This is a bug please backport to 8.1

@nielsdos nielsdos changed the base branch from master to PHP-8.1 December 23, 2022 14:27
@nielsdos
Copy link
Member Author

I've update the PR, 8.2 and master additionally need this PR: #10160

@cmb69
Copy link
Member

cmb69 commented Dec 23, 2022

I'm slightly concerned about no longer allowing new GMP(); maybe we should keep the ability to call the constructor without argument for PHP 8.1?

@Girgias
Copy link
Member

Girgias commented Dec 23, 2022

I'm slightly concerned about no longer allowing new GMP(); maybe we should keep the ability to call the constructor without argument for PHP 8.1?

I don't know, because if anyone is instantiating it via new they probably have a bug as nothing would actually work except if it's meant to be initialized to 0, which seems to be very unlikely.

@cmb69
Copy link
Member

cmb69 commented Dec 23, 2022

[…] except if it's meant to be initialized to 0, which seems to be very unlikely.

When it comes to the creativity of users, almost nothing is unlikely. After all, GMP objects can be constructed normally since they've been invented. Given that there is no need to hurry (next QAs are scheduled for Jan, 19th), it might be a good idea to ask on internals about this change (and maybe on other channels as well). Maybe I'm just too overcautious.

People using new GMP() with and without argument does seem to occur according to sourcegraph: https://sourcegraph.com/search?q=context:global+new%5CsGMP%5C%28%5Cd*%5C%29+lang:php&patternType=regexp&case=yes&sm=1

At least the first occurrences are about namespaced GMP classes, so that wouldn't affect this change. Searching for \GMP (FQN) yields no results. The truth might be somewhere in between.

@Girgias
Copy link
Member

Girgias commented Dec 28, 2022

[…] except if it's meant to be initialized to 0, which seems to be very unlikely.

When it comes to the creativity of users, almost nothing is unlikely. After all, GMP objects can be constructed normally since they've been invented. Given that there is no need to hurry (next QAs are scheduled for Jan, 19th), it might be a good idea to ask on internals about this change (and maybe on other channels as well). Maybe I'm just too overcautious.

People using new GMP() with and without argument does seem to occur according to sourcegraph: https://sourcegraph.com/search?q=context:global+new%5CsGMP%5C%28%5Cd*%5C%29+lang:php&patternType=regexp&case=yes&sm=1

At least the first occurrences are about namespaced GMP classes, so that wouldn't affect this change. Searching for \GMP (FQN) yields no results. The truth might be somewhere in between.

sigh at this point we may as well just implement the constructor properly, so one can initialize a GMP object this way. Because I find it just strange to allow instantiating it while not passing any arguments. In any case, this is going to be a nightmare to document.

@cmb69
Copy link
Member

cmb69 commented Dec 29, 2022

sigh at this point we may as well just implement the constructor properly, so one can initialize a GMP object this way. Because I find it just strange to allow instantiating it while not passing any arguments. In any case, this is going to be a nightmare to document.

Like I said, "maybe I'm just too overcautious". :)

@Girgias
Copy link
Member

Girgias commented Jan 2, 2023

From the ML discussion, it seems the preferred option is to make a proper constructor that is like gmp_init(). @nielsdos do you want to tackle this?

@nielsdos
Copy link
Member Author

nielsdos commented Jan 2, 2023

From the ML discussion, it seems the preferred option is to make a proper constructor that is like gmp_init().

That makes the most sense to me as well.

@nielsdos do you want to tackle this?

Sure, I'll do it soon-ish :)

@nielsdos
Copy link
Member Author

nielsdos commented Jan 4, 2023

Superseded by #10225

@nielsdos nielsdos closed this Jan 4, 2023
Girgias pushed a commit that referenced this pull request Jan 19, 2023
Implements a proper constructor for GMP as discussed in both GH-10158 and https://externals.io/message/119216.
Fixes GH-10155

Closes GH-10225

Signed-off-by: George Peter Banyard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GMP Objects should disallow invoking constructor like other opaque objects
3 participants