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.

Sign up for GitHub

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

Jump to bottom

dev/core#2601 fix joomla permission regression #20256

Merged
merged 1 commit into from
May 10, 2021
Merged

dev/core#2601 fix joomla permission regression #20256

merged 1 commit into from
May 10, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 9, 2021 •
edited by totten
Loading

Overview

This was a change we ultimately decided to, but forgot to, revert. It gave
extensions the ability to modify already-defined-permissons but
was brought out of scope of the change as it required more
consideration that simply adding the new super duper permission

In the meantime it caused a Joomla! regression
https://lab.civicrm.org/dev/core/-/issues/2601

Before

error

After

fixed

Technical Details

Comments

@civibot
Copy link

civibot bot commented May 9, 2021

(Standard links)

@civibot civibot bot added the 5.38 label May 9, 2021
dev/core#2601 fix joomla permission regression
34f4769 
This was a change we ultimately decided to, but forgot to, revert. It gave
extensions the ability to modify already-defined-permissons but
was brought out of scope of the change as it required more
consideration that simply adding the new super duper permission

In the meantime it caused a Joomla! regression
https://lab.civicrm.org/dev/core/-/issues/2601
@seamuslee001
Copy link
Contributor

Merging based on reports that this fixes things in the Joomla channel

@seamuslee001 seamuslee001 merged commit f173f64 into civicrm:5.38 May 10, 2021
@seamuslee001 seamuslee001 deleted the 538 branch May 10, 2021 20:51
@seamuslee001 seamuslee001 mentioned this pull request May 10, 2021
Merged
totten added a commit to totten/civicrm-core that referenced this pull request May 11, 2021
@totten
(REF) dev/core#2601 - Cleanup stale parameter
1a1100e 
Overview
--------

This cleans up a parameter that became unused/unnecessary in civicrm#20256.

History
-------

This revision is a little bouncy. Instead of before/after, here's a history:

* `<= v5.36`: `getAllModulePermissions($descriptions = FALSE)`
* `=  v5.37.0`: `getAllModulePermissions($descriptions = FALSE, &$permissions)`
* `>= v.37.1`: `getAllModulePermissions($descriptions = FALSE)`

I grepped `universe` to confirm that no other repos were using the intermediate signature.
@totten totten mentioned this pull request May 11, 2021
Merged
@totten
Copy link
Member

totten commented May 11, 2021

Just a couple quick notes:

  1. I did a grep on universe to double-check if $ufPerm->getAllModulePermissions() was being used elsewhere. There was one other reference in civihr.git, which (in any event) would be fixed by the same patch.

  2. Generally, we shouldn't expect other repos to use $ufPerm->getAllModulePermissions() or similar. Most application-logic should use APIv4's Permission.get for resolving perms. (civicrm-joomla is maybe special, but not necessarily.)

  3. There's an old reference in https://github.com/civicrm/civicrm-core/blob/7b52581/CRM/Core/Permission.php#L594. It should be innocuous, but I opened (REF) dev/core#2601 - Cleanup stale parameter #20271.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
5.38
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @seamuslee001 @totten