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

Allow extensions to specify a load priority #1838

Closed
wants to merge 4 commits into from
Closed

Allow extensions to specify a load priority #1838

wants to merge 4 commits into from

Conversation

KyrneDev
Copy link
Contributor

@KyrneDev KyrneDev commented Aug 7, 2019

This would allow a simple fix to #1832 as well as other load priority problems. I have confirmed that after using these proposed changes + adding a priority to the lock extension the issue went away.

Changes proposed in this pull request:
Allow extensions to specify an optional load "priority" in their composer.json under Flarum extension that dictates the order they are loaded in (higher means first)

Reviewers should focus on:
Is this the best way to do this?

Confirmed

  • Backend changes: tests are green (run php vendor/bin/phpunit).

Required changes:

  • Extensions such as lock need a priority set

@luceos luceos requested a review from franzliedke August 7, 2019 06:36
Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely, down the line, we'll want to implement a more robust system that avoids issues like this without using mere numbers, ex by specifying if the extension has to load before and/or after extension(s).

For now though, I guess this should be fine, and it is similar to the initializer logic on the JS side.

@franzliedke
Copy link
Contributor

I'd like to think about this some more. If I have no better idea, we can merge it before the next beta release.

@franzliedke
Copy link
Contributor

franzliedke commented Sep 10, 2019

I cannot say I like this very much, because it does not tackle the root cause (the tags extension overwrites the fallback permission check, lock adds an additional performance permission "middleware", so to speak - maybe we should differentiate these concepts?).

I can revisit this when creating an extender for the permission system.

Leaving for @luceos to review and merge.

@luceos
Copy link
Member

luceos commented Sep 10, 2019

I dislike this approach too, it bloats the composer.json file unnecessarily. I rather go with @franzliedke proposal right now and do this properly from the get go.

Sorry, but this feels like one of those PR's that might haunt us in the long run.

@luceos luceos closed this Sep 10, 2019
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