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

[fix] broken events before and after save on JModelAdmin. Related #4308 #5203

Closed
wants to merge 2 commits into from
Closed

[fix] broken events before and after save on JModelAdmin. Related #4308 #5203

wants to merge 2 commits into from

Conversation

phproberto
Copy link
Contributor

This PR broke event B/C #4308

The default context passed to plugins on JModelAdmin save was:

$this->option . '.' . $this->name`

and it was replaced by:

$context = $this->option . '_' . $this->name;

That breaks all the existing plugins triggered before and after save events.

See:
https://github.com/joomla/joomla-cms/pull/4308/files#diff-39b4bc5cb02bbace2ee4febe8f54583eR1079

@phproberto
Copy link
Contributor Author

Please do not merge because I want to create a getContext() function that:

  • Will avoid this small errors in the future
  • Will allow us to override the context

@amazeika
Copy link
Contributor

@phproberto Thank you for noticing and reporting the issue with the typo that got introduced on our PR #4308. We've just submitted a PR for fixing this exact issue (see #5205). Thank you again.

@Bakual
Copy link
Contributor

Bakual commented Nov 25, 2014

I'm closing this PR since I already merged the fix by @amazeika and said to not merge it yet anyway.

@phproberto Feel free to reopen if it's ready.

@Bakual Bakual closed this Nov 25, 2014
@phproberto
Copy link
Contributor Author

Thanks @amazeika for the fast fix :) and thanks @Bakual for merging it. I'll prepare a new PR with the context changes which is much better because now the issue is already fixed.

@amazeika
Copy link
Contributor

@phproberto No probs :). I really think that having the fix pushed first was very important. @Bakual did a great job on merging it almost instantly. The proposition you've made for the context getter is a good idea. This will certainly help on avoiding small errors with big consequences such as this.

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.

4 participants