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

Deprecate internal SplFileInfo::_bad_state_ex() method #8318

Merged
merged 1 commit into from Jun 9, 2022

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Apr 7, 2022

Use standard VM handling instead

Not exactly sure how to write a test for this.

@cmb69
Copy link
Contributor

cmb69 commented Apr 7, 2022

Hmm, removing a public function (even when prefixed with _ and undocumented) in a minor version doesn't look like a good idea (although that method might better not have been introduced in the first place).

@Girgias
Copy link
Member Author

Girgias commented Apr 7, 2022

I would be very surprised if someone actually uses this, it also makes for a very confusing back trace.

But I suppose it's always possible to just deprecate the method and remove it in PHP 9.

@jurchiks
Copy link

jurchiks commented Apr 7, 2022

Hmm, removing a public function (even when prefixed with _ and undocumented) in a minor version doesn't look like a good idea (although that method might better not have been introduced in the first place).

Why was this function even public in the first place? It's obviously internal use only, not to mention completely redundant as it's only really used in one place. Stuff like this shouldn't be allowed in code in the first place, and if it's already there, it should be gotten rid of ASAP, it's just bad code smell.

@javiereguiluz
Copy link
Contributor

Here are all the occurrences of this method in the entire GitHub codebase, in case you want to check if any of those usages is legit:

https://github.com/search?l=PHP&p=1&q=_bad_state_ex&type=Code

@jurchiks
Copy link

jurchiks commented Apr 7, 2022

Ouch. Now if only there was a way to filter by filename ext/spl/spl_directory.stub.php, which are all clones of php-src, along with some other common stub file names...
Then we'd see the real picture.

func = zend_std_get_method(object, tmp, NULL);
zend_string_release_ex(tmp, 0);
return func;
zend_throw_error(NULL, "The parent constructor was not called: the object is in an invalid state");
Copy link
Member

Choose a reason for hiding this comment

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

We'll probably want to move this check into each method and drop the get_method overload. That way the stack trace should be correct. Can you adjust the test to also check stack trace?

Copy link
Member Author

Choose a reason for hiding this comment

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

So I tried to do this, but because userland can extend the class and rewrite the method, this check will not be done if moved into each function, that's why I think it's a get handler and I don't know how to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the method final?

Copy link
Member Author

Choose a reason for hiding this comment

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

This method is final, but I was talking about how other methods can be overwritten and thus bypass a check we could introduce in the method to see if the object is properly initialised. And I think the get_method overload checks that a userland class can't put the object in an invalid state (which mainly seems to be an issue for the GlobIterator)

@Girgias Girgias force-pushed the spl-remove-internal-method branch 2 times, most recently from c017432 to ce5aa82 Compare April 27, 2022 18:56
@ramsey
Copy link
Member

ramsey commented May 24, 2022

I get 32 results when I query for _bad_state_ex filename:"spl_directory.stub.php" language:PHP

https://github.com/search/advanced?q=_bad_state_ex+filename%3A%22spl_directory.stub.php%22&type=Code

So, that stands to reason that the results of _bad_state_ex language:PHP minus 32 would be closer to the actual number:

https://github.com/search?q=_bad_state_ex+language%3APHP&type=Code

So, it's somewhere around 5,000.

Many of those still appear to be stubs of some form or another.

@ramsey ramsey added this to the PHP 8.2 milestone May 24, 2022
@Girgias Girgias force-pushed the spl-remove-internal-method branch from ce5aa82 to 65da53f Compare June 8, 2022 12:12
@Girgias Girgias changed the title Remove internal SplFileInfo::_bad_state_ex() method Deprecate internal SplFileInfo::_bad_state_ex() method Jun 8, 2022
Use standard VM handling instead

Deprecate the method
@Girgias Girgias force-pushed the spl-remove-internal-method branch from 65da53f to c4f8eb5 Compare June 8, 2022 12:53
@Girgias Girgias merged commit dbf1caf into php:master Jun 9, 2022
@Girgias Girgias deleted the spl-remove-internal-method branch June 9, 2022 12:25
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.

None yet

6 participants