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

Fixed bug in sendNotificationMessage Hook processing #79

Merged
merged 1 commit into from
Oct 14, 2015
Merged

Fixed bug in sendNotificationMessage Hook processing #79

merged 1 commit into from
Oct 14, 2015

Conversation

Defcon0
Copy link
Contributor

@Defcon0 Defcon0 commented Sep 14, 2015

The object had never been called in the former implementation

The object had never been called in the former implementation
@aschempp
Copy link
Member

Looks good to me.

@Toflar
Copy link
Member

Toflar commented Sep 14, 2015

  1. If importStatic() then obviously System::importStatic(), not Controller::importStatic().
  2. @dmolineus: As this was your contribution, do you want to check this?

@dmolineus
Copy link
Contributor

@Toflar I can't reproduce why the previous implementation does not work. It works pretty fine here. But I don't mind using the importStatic method here.

@Defcon0
Copy link
Contributor Author

Defcon0 commented Sep 25, 2015

That would be great. If you need another reason ro use the proposed solution, it's contaos standard way to handle hook calls.

@Toflar
Copy link
Member

Toflar commented Sep 28, 2015

So I can close this, right? It's not a bug and importStatic is actually not a good choice anyway.

@Defcon0
Copy link
Contributor Author

Defcon0 commented Sep 28, 2015

Why isn't importStatic a good choice? In addition I didn't see the way it's implemented right now in the whole core before.

@Toflar
Copy link
Member

Toflar commented Sep 28, 2015

Why isn't importStatic a good choice?

Because it's a useless dependency on System and I never understood what it should be good for :)

@Toflar Toflar closed this Sep 28, 2015
@Toflar Toflar added the invalid label Sep 28, 2015
@dmolineus
Copy link
Contributor

Just thought about it one more time. The current implementation won't work with classes using the getInstance singleton "standard" of Contao. But that could be a known limitation. ;)

@Toflar
Copy link
Member

Toflar commented Sep 28, 2015

Uff, okay then we could change it (even though I don't like it) but not Controller:: but System:: instead.

@Toflar Toflar reopened this Sep 28, 2015
@Toflar Toflar added this to the 1.3.1 milestone Sep 28, 2015
@Defcon0
Copy link
Contributor Author

Defcon0 commented Sep 28, 2015

Mhm, I just gave it another try and it simply doesn't work. call_user_func doesn't call the created object's method. Would be kind, if someone could try to reproduce it with the module which doesn't work with the current implementation. It's one of our modules: https://github.com/heimrichhannot/contao-notification_center_plus (also available via composer). It adds css inlining and an additional salutation shorthand token. And addTokens in assigned as a hook in config.php doesn't work. When importing it static in the way I described above, it works like a charm.

@aschempp aschempp modified the milestones: 1.3.2, 1.3.1 Oct 12, 2015
@aschempp aschempp merged commit 0a96ca7 into terminal42:master Oct 14, 2015
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.

4 participants