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

[BUG]: Null cannot be set in the source parameter when firing an event. #15133

Closed
fagai opened this issue Aug 20, 2020 · 9 comments · Fixed by #15710
Closed

[BUG]: Null cannot be set in the source parameter when firing an event. #15133

fagai opened this issue Aug 20, 2020 · 9 comments · Fixed by #15710
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release breaks bc Functionality that breaks Backwards Compatibility bug A bug report documentation Documentation required status: low Low

Comments

@fagai
Copy link

fagai commented Aug 20, 2020

Describe the bug

Null cannot be set in the source parameter when firing an event.
There is an example using null in the documentation.
https://docs.phalcon.io/4.0/ja-jp/events#responses

Also, you cannot specify a character string.
There are many examples of using $this in the documentation, but I put values ​​such as user_id.

c4cf1db

To Reproduce

Phalcon\Events\Exception: The source of register event must be an object, got NULL"
Phalcon\Events\Exception: The source of register event must be an object, got string

Steps to reproduce the behavior:

$eventsManager->fire('custom:custom', null);

Provide minimal script to reproduce the issue

<?php

use Phalcon\Events\Manager as EventsManager;

$eventsManager = new EventsManager();

$eventsManager->attach(
    'custom:custom',
    function () {
        return 'first response';
    }
);

$eventsManager->fire('custom:custom', null);

Expected behavior
A clear and concise description of what you expected to happen.

No Error

Details

  • Phalcon version: 4.0.5
  • PHP Version: 7.2.30
  • Operating System: alpine linux
  • Installation type: Compiling from source
  • Zephir version (if any):
  • Server: Apache+php-fpm
  • Other related info (Database, table schema):
@fagai fagai added bug A bug report status: unverified Unverified labels Aug 20, 2020
@zsilbi zsilbi added 6.0 The issues we want to solve in the 6.0 release breaks bc Functionality that breaks Backwards Compatibility and removed status: unverified Unverified labels Sep 10, 2020
@zsilbi zsilbi added the transfer label Oct 6, 2020
@fagai
Copy link
Author

fagai commented Nov 4, 2020

@zsilbi This issue worked in 3.4.

@Jeckerson Jeckerson added 5.0 The issues we want to solve in the 5.0 release and removed 6.0 The issues we want to solve in the 6.0 release transfer labels Mar 30, 2021
@Izopi4a
Copy link
Member

Izopi4a commented May 4, 2021

i don't think this would have workerd in any 3.x version. Based on this : https://github.com/phalcon/cphalcon/blob/3.0.x/phalcon/events/manager.zep#L376 2nd parameter is always the component that is listening for the event.
the 3rd param ( data ) can be null. Seems like the docs are wrong and needs to be updated

@Izopi4a Izopi4a added the documentation Documentation required label May 4, 2021
@fagai
Copy link
Author

fagai commented May 4, 2021

In the service I was developing, I have confirmed that it worked without problems even if the second argument was null. (v3.4.5)

I think the problem occurred because the following judgment was added.
https://github.com/phalcon/cphalcon/blob/4.0.x/phalcon/Events/Event.zep#L72-L76

@Izopi4a
Copy link
Member

Izopi4a commented May 4, 2021

ok indeed in 3.4.x is working. for me it is

Version | 3.4.4 //phalcon
Powered by Zephir | Version 0.10.16-6826149172

but i think this issue is because of zephir-lang/zephir#1656. This was merged in zephir 0.11+ and we used zephir 0.10 to build phalcon 3.4.X because it was breaking everything. before that zephir wasn't respecting params and return types correctly.

for me we only need to update the docs, I don't think there is a bug here.

also I am able to reproduce this error:

 [TypeError] Phalcon\Events\Manager::fire(): Argument #2 ($source) must be of type object, null given

and not exactly what you have shown. but I am pretty sure it is the same case.

@fagai
Copy link
Author

fagai commented May 4, 2021

I see.

I had this problem because I sometimes didn't use the second argument.
Currently, we are dealing with it by passing an empty object. So if it's in the right direction, that's fine.

@Jeckerson
Copy link
Member

I'm starting to see some sense to 2nd argument source be nullable:

object source = null

@niden Could you provide more information if my thoughts makes sense?

@niden
Copy link
Sponsor Member

niden commented Oct 5, 2021

I looked at the manager code and making the source nullable can be done. The manager can fire the event when the source is null

let status = call_user_func_array(
    handler,
    [event, source, data]
);

We can make this change in v5 since it will break BC.

@niden
Copy link
Sponsor Member

niden commented Oct 5, 2021

Personally I never had to send null to the source; I always sent the actual object that was using this code.

@niden niden added this to Working on it in Phalcon Roadmap Oct 6, 2021
@niden niden added the status: low Low label Oct 6, 2021
@niden niden mentioned this issue Oct 6, 2021
5 tasks
@niden niden linked a pull request Oct 6, 2021 that will close this issue
5 tasks
@niden
Copy link
Sponsor Member

niden commented Oct 6, 2021

Resolved in #15710

Thank you @fagai

@niden niden closed this as completed Oct 6, 2021
Phalcon Roadmap automation moved this from Working on it to Implemented Oct 6, 2021
@niden niden moved this from Implemented to Released in Phalcon Roadmap Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release breaks bc Functionality that breaks Backwards Compatibility bug A bug report documentation Documentation required status: low Low
Projects
Archived in project
Phalcon Roadmap
  
Released
Development

Successfully merging a pull request may close this issue.

5 participants