session: fix create_sid()/validateId() check depending on interface order - #23329
session: fix create_sid()/validateId() check depending on interface order#23329lazerg wants to merge 3 commits into
Conversation
|
The Alpine ASAN failure is ext/mysqli/tests/protocol_stmt_row_fetch_data.phpt, which fails on master too. It is not caused by this change. |
| for (uint32_t i = 0; i < ce->num_interfaces; i++) { | ||
| if (zend_hash_str_exists(&ce->interfaces[i]->function_table, name, name_len)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd rather we iterate on ce->interfaces[i] and check that the CE is SessionIdInterface or SessionUpdateTimestampHandlerInterface rather than name checking.
There was a problem hiding this comment.
Done in a783cff. It iterates ce->interfaces[i] and uses instanceof_function() per entry, so an interface that extends SessionIdInterface or SessionUpdateTimestampHandlerInterface is still recognised before it has been flattened into the list.
One case this drops, which the name check covered: a class that gets the methods from an unrelated interface. implements OwnMethods, SessionHandlerInterface stays quiet, implements SessionHandlerInterface, OwnMethods now warns again. session_set_save_handler() accepts the methods there regardless of the interface, so the two rules differ. Tell me if you want that case back and I will restore the method lookup.
There was a problem hiding this comment.
Considering the objective is to move the methods from those 2 interfaces to the "base" interface having another interface declare such a method is not something we should be supporting.
There was a problem hiding this comment.
please move this test into the user_session_submodule subdirectory
There was a problem hiding this comment.
Moved to ext/session/tests/user_session_module/gh23328.phpt in a783cff.
session_handler_interface_gets_implemented()runs whileSessionHandlerInterfaceis being attached to the class, soclass->function_tableonly holds what the class itself declares. Interfaces listed after it have not contributed their abstract methods yet, which is why the warning fires or not depending on the order of theimplementslist.The check now also looks for the two methods in the function tables of the interfaces the class implements, which
zend_do_implement_interfaces()fills in before it starts calling the handlers. Staying on method presence rather than interface identity keeps it consistent withsession_set_save_handler(), which accepts the methods whether or not the interface is implemented.Closes #23328