Skip to content

Disallow invoking GMP constructor - #10158

Closed
ndossche wants to merge 1 commit into
php:PHP-8.1from
ndossche:gmp-construct
Closed

Disallow invoking GMP constructor#10158
ndossche wants to merge 1 commit into
php:PHP-8.1from
ndossche:gmp-construct

Conversation

@ndossche

@ndosschendossche commented Dec 22, 2022

Copy link
Copy Markdown
Member

FixesGH-10155

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

@Girgias

Copy link
Copy Markdown
Member

This is a bug please backport to 8.1

@ndossche
ndossche changed the base branch from master to PHP-8.1December 23, 2022 14:27
@ndossche

Copy link
Copy Markdown
MemberAuthor

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

@cmb69

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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
Copy Markdown
Member

[…] 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
Copy Markdown
Member

[…] 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
Copy Markdown
Member

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
Copy Markdown
Member

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?

@ndossche

Copy link
Copy Markdown
MemberAuthor

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 :)

@ndosschendossche mentioned this pull request Jan 4, 2023
@ndossche

Copy link
Copy Markdown
MemberAuthor

Superseded by #10225

@ndosschendossche 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.
FixesGH-10155ClosesGH-10225
Signed-off-by: George Peter Banyard <girgias@php.net>
Sign up for freeto 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

@ndossche@Girgias@cmb69