# OO Parameter Checking

- Thread: https://externals.io/message/35863
- Original archive: https://news-web.php.net/php.internals/35863
- Started by Christian Schneider on 2008-02-29T17:27:12+00:00
- Messages: 5
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #35863 — Christian Schneider — 2008-02-29T17:27:12+00:00

URL: https://externals.io/message/35863#35863

Marcus Boerger wrote:
>   you still cannot ignore basic inheritance or reuse rules. Protocols
> have to be respected -> E_FATAL, fix your code.

I have several comments (plus patches) to this:

1) The current checks are IMHO too strict regarding default values for
parameters: An inheriting class can add default values to a parameter
without breaking the protocol:
class A { function foo($x) { ... } }
class B extends A { function foo($x = 42) { ... } }
should be allowed. Only if there are fewer parameters in B::foo or if
B::foo has more *required* parameters than A::foo the protocol is
broken. Otherwise every instance of B can be passed to something
expecting A.
The attached paramcheck_relaxed.patch.txt for HEAD changes this.

2) The current checks are IMHO too strict regarding static functions:
Static functions are not part of an instance's protocol (they can not be
passed around) and should be left out of the check the same way
constructors are ignored. The use case for this is a factory method:
class A { static function factory($size) { ... } }
class B extends A { static function factory($size, $color) { ... } }
The attached paramcheck_relaxed.patch.txt for HEAD changes this.

3) I guess more controversial is the change from `E_STRICT` to
 `E_COMPILE_ERROR` for PHP 6 regarding these checks. While you consider
such OO code as broken I would highly appreciate it if you recognize
that other people have other coding standards and/or old code to
maintain. Adhering to the
http://en.wikipedia.org/wiki/Liskov_substitution_principle is something
strict OO coders want to do. And they are using `E_STRICT` anyway, so I
see no reason to force everybody else to change their code, so it should
be left a simple `E_STRICT` IMHO.
The attached paramcheck_e_strict.patch for HEAD changes this back to the
old behaviour.

If paramcheck_relaxed.patch.txt gets accepted I'd volunteer to back-port
it to 5.x as well.

Regards,
- Chris

---

## #35865 — Stanislav Malyshev — 2008-02-29T17:44:44+00:00

URL: https://externals.io/message/35863#35865 · In reply to: #35863

Hi!

> 1) The current checks are IMHO too strict regarding default values for
> parameters: An inheriting class can add default values to a parameter
> without breaking the protocol:
> class A { function foo($x) { ... } }
> class B extends A { function foo($x = 42) { ... } }
> should be allowed. Only if there are fewer parameters in B::foo or if

I think this makes sense. In fact, if B::foo() has less parameters it 
might be OK too, since B would just ignore extra parameters (and in fact 
it could do it anyway :)

> 2) The current checks are IMHO too strict regarding static functions:
> Static functions are not part of an instance's protocol (they can not be
> passed around) and should be left out of the check the same way
> constructors are ignored. The use case for this is a factory method:
> class A { static function factory($size) { ... } }
> class B extends A { static function factory($size, $color) { ... } }

Not sure about this - might be some nasty surprises here if you 
substitute class B for class A. Anyway, why not call B's function 
factoryB or something? If you are only going to call it directly by 
name, there's no difference.
It would be nice if you made separate patches for each feature.
-- 
Stanislav Malyshev, Zend Software Architect
stas@zend.com   http://www.zend.com/
(408)253-8829   MSN: stas@zend.com

---

## #35864 — Robert Cummings — 2008-02-29T17:44:50+00:00

URL: https://externals.io/message/35863#35864 · In reply to: #35863

> Marcus Boerger wrote:
> > […]

I don't see why THIS protocol should need to be respected when PHP lacks
argument signature support (thus allowing method overloading). This used
to be something that was flexible in PHP4 but now you can't modify the
args signature downstream despite the obvious lack of method
overloading. Shackles abound that once didn't exist. Using
 `func_get_args()` to simulate this is a gut wrenching experience of sloppy
necessity. "Fix your code" isn't very constructive... one could fire
back "fix the engine" :/

Cheers,
Rob.



> 
> I have several comments (plus patches) to this:
> 
> 1) The current checks are IMHO too strict regarding default values for
> parameters: An inheriting class can add default values to a parameter
> without breaking the protocol:
> class A { function foo($x) { ... } }
> class B extends A { function foo($x = 42) { ... } }
> should be allowed. Only if there are fewer parameters in B::foo or if
> B::foo has more *required* parameters than A::foo the protocol is
> broken. Otherwise every instance of B can be passed to something
> expecting A.
> The attached paramcheck_relaxed.patch.txt for HEAD changes this.
> 
> 2) The current checks are IMHO too strict regarding static functions:
> Static functions are not part of an instance's protocol (they can not be
> passed around) and should be left out of the check the same way
> constructors are ignored. The use case for this is a factory method:
> class A { static function factory($size) { ... } }
> class B extends A { static function factory($size, $color) { ... } }
> The attached paramcheck_relaxed.patch.txt for HEAD changes this.
> 
> 3) I guess more controversial is the change from `E_STRICT` to
> `E_COMPILE_ERROR` for PHP 6 regarding these checks. While you consider
> such OO code as broken I would highly appreciate it if you recognize
> that other people have other coding standards and/or old code to
> maintain. Adhering to the
> http://en.wikipedia.org/wiki/Liskov_substitution_principle is something
> strict OO coders want to do. And they are using `E_STRICT` anyway, so I
> see no reason to force everybody else to change their code, so it should
> be left a simple `E_STRICT` IMHO.
> The attached paramcheck_e_strict.patch for HEAD changes this back to the
> old behaviour.
> 
> If paramcheck_relaxed.patch.txt gets accepted I'd volunteer to back-port
> it to 5.x as well.
> 
> Regards,
> - Chris
> plain text document attachment (paramcheck_relaxed.patch.txt)
> Index: Zend/zend_compile.c
> ===================================================================
> RCS file: /repository/ZendEngine2/zend_compile.c,v
> retrieving revision 1.804
> diff -u -r1.804 zend_compile.c
> --- Zend/zend_compile.c	23 Feb 2008 21:24:18 -0000	1.804
> +++ Zend/zend_compile.c	29 Feb 2008 16:26:51 -0000
> @@ -2477,12 +2477,12 @@
>  	}
>  
>  	/* Checks for constructors only if they are declared in an interface */
> -	if ((fe->common.fn_flags & ZEND_ACC_CTOR) && !(proto->common.scope->ce_flags & ZEND_ACC_INTERFACE)) {
> +	if ((fe->common.fn_flags & (ZEND_ACC_CTOR | ZEND_ACC_STATIC)) && !(proto->common.scope->ce_flags & ZEND_ACC_INTERFACE)) {
>  		return 1;
>  	}
>  
>  	/* check number of arguments */
> -	if (proto->common.required_num_args != fe->common.required_num_args
> +	if (proto->common.required_num_args < fe->common.required_num_args
>  		|| proto->common.num_args > fe->common.num_args) {
>  		return 0;
>  	}
> plain text document attachment (paramcheck_e_strict.patch.txt)
> Index: Zend/zend_compile.c
> ===================================================================
> RCS file: /repository/ZendEngine2/zend_compile.c,v
> retrieving revision 1.804
> diff -u -r1.804 zend_compile.c
> --- Zend/zend_compile.c	23 Feb 2008 21:24:18 -0000	1.804
> +++ Zend/zend_compile.c	29 Feb 2008 17:23:45 -0000
> @@ -2617,7 +2617,7 @@
>  		child->common.prototype = parent->common.prototype ? parent->common.prototype : parent;
>  	}
>  
> -	if (child->common.prototype) {
> +	if (child->common.prototype && (child->common.prototype->common.fn_flags & ZEND_ACC_ABSTRACT)) {
>  		if (!zend_do_perform_implementation_check(child, child->common.prototype TSRMLS_CC)) {
>  			zend_error(E_COMPILE_ERROR, "Declaration of %v::%v() must be compatible with that of %v::%v()", ZEND_FN_SCOPE_NAME(child), child->common.function_name, ZEND_FN_SCOPE_NAME(child->common.prototype), child->common.prototype->common.function_name);
>  		}
> 
> -- 
> 
> 
-- 
.------------------------------------------------------------.
| InterJinn Application Framework - http://www.interjinn.com |
:------------------------------------------------------------:
| An application and templating framework for PHP. Boasting  |
| a powerful, scalable system for accessing system services  |
| such as forms, properties, sessions, and caches. InterJinn |
| also provides an extremely flexible architecture for       |
| creating re-usable components quickly and easily.          |
`------------------------------------------------------------'

---

## #35869 — Marcus Boerger — 2008-03-01T12:53:39+00:00

URL: https://externals.io/message/35863#35869 · In reply to: #35865

Hello Christian, Stanislav,

Friday, February 29, 2008, 6:44:44 PM, you wrote:

> Hi!

> > […]

> I think this makes sense. In fact, if B::foo() has less parameters it 
> might be OK too, since B would just ignore extra parameters (and in fact 
> it could do it anyway :)

Sounds fine to me.

Originally I thought of allowing fully correct protocol checks that would
have allowed to even change the type hints, as long as all rules are
followed. But we decided against this and instead went with a very strict
approach. Maybe it is time to limit the strictness and apply that part of
the patch.

> > […]

> Not sure about this - might be some nasty surprises here if you 
> substitute class B for class A. Anyway, why not call B's function 
> factoryB or something? If you are only going to call it directly by 
> name, there's no difference.

The problem here starts with late static binding. So I think we need to
stay with the inheritance/protocol stack.


Best regards,
 Marcus

---

## #35870 — Marcus Boerger — 2008-03-01T13:56:49+00:00

URL: https://externals.io/message/35863#35870 · In reply to: #35869

Hello Christian,

Saturday, March 1, 2008, 1:53:39 PM, you wrote:

> Hello Christian, Stanislav,

> Friday, February 29, 2008, 6:44:44 PM, you wrote:

> > […]

> > […]

> > […]

> Sounds fine to me.

> Originally I thought of allowing fully correct protocol checks that would
> have allowed to even change the type hints, as long as all rules are
> followed. But we decided against this and instead went with a very strict
> approach. Maybe it is time to limit the strictness and apply that part of
> the patch.
Johannes, who is sitting next to me, wrote a test and committed that part.

Best regards,
 Marcus