Skip to content

Conversation

pmmaga
Copy link
Contributor

@pmmaga pmmaga commented Jun 30, 2016

When an implementation of a child method has the same name as a private parent method, their signatures are checked for compatibility.

This shouldn't be the case as if the parent's method is private, the child's method is not overriding it.

Link for the bug: https://bugs.php.net/bug.php?id=72496

PS: I also have the fix for master. Should I create a different PR for that? It won't be possible to apply this PR against it. The code moved from zend_compile to zend_inheritance.

bukka and others added 30 commits January 25, 2016 16:50
…setup

Only needed with Apache version < 2.4.12 (ex RHEL-7)
This reverts commit fa548e5.
Also fix some CS issue and naming
* PHP-5.5:
  Upgrade bundled PCRE to 8.38
  Fixed NEWS file entry
  fix the fix for bug #70976 (imagerotate)
* origin/PHP-5.6.18:
  prepare 5.6.18RC1
  Fix test when run with openssl < 1.0.2 (reorder so no more SSLv2 message) Fix skip message to work
  improve fix for bug #71201
  fork test
  fix test
  fork test
  fork test for win32
  fork test
  Use SUCCESS/FAILURE
  Fixed bug #65720 ext/mbstring/libmbfl/filters/mbfilter_cp5022x.c:281: bad if test
  Fix header file include
  Fixed bug #69111 (Crash in SessionHandler::read()). Made session save handler abuse much harder than before.
* PHP-5.5:
  Upgrade bundled PCRE to 8.38
  Fixed NEWS file entry
  fix the fix for bug #70976 (imagerotate)
* PHP-5.5.32:
  Fixed bug #71488: Stack overflow when decompressing tar archives
  update NEWS
  add missing headers for SIZE_MAX
  backport the escapeshell* functions hardening branch
  add tests
  Fix bug #71459 - Integer overflow in iptcembed()
  Fixed bug #71323 - Output of stream_get_meta_data can be falsified by its input
  Fix bug #71391: NULL Pointer Dereference in phar_tar_setupmetadata()
  Fix bug #71335: Type Confusion in WDDX Packet Deserialization
  Fix bug #71354 - remove UMR when size is 0
* PHP-5.5:
  fix tests
  fix NEWS
  update NEWS
* PHP-5.6.18:
  fix tests
  fix NEWS
  Update NEWS
  update NEWS
  Fixed bug #71488: Stack overflow when decompressing tar archives
  update NEWS
  add missing headers for SIZE_MAX
  backport the escapeshell* functions hardening branch
  add tests
  Fix bug #71459 - Integer overflow in iptcembed()
  prepare 5.6.18RC1
  Fixed bug #71323 - Output of stream_get_meta_data can be falsified by its input
  Fix bug #71391: NULL Pointer Dereference in phar_tar_setupmetadata()
  Fixed bug #71331 - Uninitialized pointer in phar_make_dirstream()
  Fix bug #71335: Type Confusion in WDDX Packet Deserialization
  Fix bug #71354 - remove UMR when size is 0

Conflicts:
	configure.in
	main/php_version.h
smalyshev and others added 16 commits June 21, 2016 00:27
* PHP-5.5:
  Now the right bug #
  Fix NEWS
* PHP-5.5:
  remove the huge test file, generate it on the fly instead
This reverts commit c5d9c50.
There is a difference between TS and NTS warning message, since
virtual_mkdir vs glibc directly is used. This has no effect for
the actual fix functionality.
Obiously, it isn't sufficient to call sqlite3_clear_bindings() alone, but
also the bound_params of the php_sqlite3_stmt have to be cleared.
* PHP-5.5:
  Fix the fix for #72403 on nl2br
  5.5.38 now

Conflicts:
	configure.in
	main/php_version.h

class Bar extends Foo
{
public function getName($extraArgument)
Copy link
Contributor

@staabm staabm Jun 30, 2016

Choose a reason for hiding this comment

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

does the testsuite already contain a test for the opposite case in which the method is overriden and a warning is expected?

If not would be great to also add one with this PR to verify we dont regress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The normal inheritance tests (tests/classes/inheritance_003.phpt & tests/classes/inheritance_004.phpt) already cover the regular case.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem also arises when the function is protected, can you add a test for that ?

@greg0ire
Copy link
Contributor

I think you should squash both commits together : the test can't live without the fix, and the fix shouldn't live without the test, right?

@pmmaga
Copy link
Contributor Author

pmmaga commented Jun 30, 2016

Actually the fix can live without the test. :) But if it is really necessary i can squash them.

@greg0ire
Copy link
Contributor

greg0ire commented Jun 30, 2016

Actually the fix can live without the test. :) But if it is really necessary i can squash them.

That's why I said "should" 😉 You're not supposed to write code without tests, right? And you're supposed to write tests first, also… So let's not separate this commits otherwise every contribution ever will be :

  • do this
  • add tests
  • add docs

@greg0ire
Copy link
Contributor

greg0ire commented Jun 30, 2016

Looks good to (squash-)merge, thanks a lot @pmmaga !

@pmmaga
Copy link
Contributor Author

pmmaga commented Jun 30, 2016

Squashed the commits as well.

@cmb69
Copy link
Member

cmb69 commented Jul 2, 2016

Thanks for the PR! However, PHP 5.5 gets security fixes only, so this should go to PHP 5.6+. Can you please rebase onto PHP-5.6, Pedro?

@pmmaga
Copy link
Contributor Author

pmmaga commented Jul 3, 2016

Due to my poor rebasing skills, I've created a new PR #1980 for the fix against 5.6. Given that, I'm closing this one.

@pmmaga pmmaga closed this Jul 3, 2016
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.