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

Additional Middleware Extender functionality #1971

Closed

Conversation

@tankerkiller125
Copy link
Member

tankerkiller125 commented Jan 18, 2020

Fixes #1957

Changes proposed in this pull request:

  • Implements the remove, insertBefore, insertAfter and replace functionality for middlewares
  • Changed the original flarum.{frontend}.middleware container to array
  • Moved middleware piping to flarum.{frontend}.handler container

Reviewers should focus on:

  • General code style (hopefully StyleCI catches most of it)
  • Variable names (could we name them better?)
  • Does anyone see a better way to handle the foreach loops?

Confirmed

  • Backend changes: tests are green (run composer test).
StyleCI Fixes
@tankerkiller125 tankerkiller125 force-pushed the tankerkiller125:mk/middleware-extender branch from 5582dc5 to dc8614d Jan 18, 2020
@tankerkiller125 tankerkiller125 requested a review from franzliedke Jan 18, 2020
@tankerkiller125 tankerkiller125 changed the title Additional Middleware Extender functionality [WIP] Additional Middleware Extender functionality Jan 18, 2020
@tankerkiller125 tankerkiller125 changed the title [WIP] Additional Middleware Extender functionality Additional Middleware Extender functionality Jan 18, 2020
@tankerkiller125 tankerkiller125 requested a review from luceos Jan 18, 2020
@luceos
luceos approved these changes Jan 20, 2020
Copy link
Member

luceos left a comment

Just ran into the stubbornness of the middleware myself today; having this would be ace! Just noticed it might do with a test?

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Jan 20, 2020

@luceos I agree that a test would be beneficial however I have zero experience writing them (especially outside of the full laravel Framework) so if I do write test I'll need help from someone who know how.

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Jan 21, 2020

I'm fine with doing a test after merging, awaiting the second reviewer.

@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 21, 2020

Great job, thank you!

I agree, we can start work on testing extenders in a next step. IIRC, that was actually my suggestion for your next task. I'd happy to do another round of pairing in order to get this started. 😃

@franzliedke franzliedke mentioned this pull request Jan 24, 2020
2 of 2 tasks complete
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 24, 2020

I took care of the rebase and squash-merged this. Thank you!!!

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Jan 25, 2020

Sorry about not doing it myself, this week at work was crazy (company meetings, remote employees visiting, etc.)

@tankerkiller125 tankerkiller125 deleted the tankerkiller125:mk/middleware-extender branch Feb 4, 2020
luceos added a commit that referenced this pull request Feb 4, 2020
Implements the remove, insertBefore, insertAfter and replace
functionality for middlewares.

The IoC container now holds one array of middleware (bindings) per
frontend stack - the extender operates on that array, before it is
wrapped in a middleware "pipe".

Fixes #1957, closes #1971.
wzdiyb added a commit to wzdiyb/core that referenced this pull request Feb 16, 2020
Implements the remove, insertBefore, insertAfter and replace
functionality for middlewares.

The IoC container now holds one array of middleware (bindings) per
frontend stack - the extender operates on that array, before it is
wrapped in a middleware "pipe".

Fixes flarum#1957, closes flarum#1971.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.