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

Middleware extender #1952

Merged
merged 8 commits into from Dec 12, 2019
Merged

Middleware extender #1952

merged 8 commits into from Dec 12, 2019

Conversation

@tankerkiller125
Copy link
Contributor

tankerkiller125 commented Dec 8, 2019

Fixes #1919

Changes proposed in this pull request:
Adds the Middleware extender for the extension API

Reviewers should focus on:

  • General Code Quality
  • Does this meet the general requirements of the extender? (Zend Middlewares do not have a way to organize or remove middleware from my reading)
  • Docblocks? (Other Extenders lack Docblocks)

Screenshot

No end user changes, extension developers can now use the following in extend.php:

(new Extend\Middleware('forum'))->add(FirstMiddleware::class),
(new Extend\Middleware('forum'))->add(SecondMiddleware::class)
// OR
(new Extend\Middleware('forum'))->add(FirstMiddleware::class)->add(SecondMiddleware::class)

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
Sync with Flarum
Merge original
Merge original
Merge Flarum
Fixed Syleing


Array should be short type


Fixed Style
Copy link
Member

luceos left a comment

Looking great Matthew, one nitpick comment from my side 👍

src/Extend/Middleware.php Outdated Show resolved Hide resolved
tankerkiller125 and others added 2 commits Dec 8, 2019
Co-Authored-By: Daniël Klabbers <luceos@users.noreply.github.com>
@luceos
luceos approved these changes Dec 8, 2019
@luceos

This comment has been minimized.

Copy link
Member

luceos commented Dec 8, 2019

Related to your docblock question. My reasoning usually is this:

If the method name, arguments and return type hints are obvious enough, no docblock might be necessary. Sometimes you might need to clarify things, especially when the code could be used by lesser experienced users. Adding middleware, in my opinion, is pretty high level, so the way you wrote it works fine for me.

@datitisev

This comment has been minimized.

Copy link
Member

datitisev commented Dec 8, 2019

We might want to make the class properties protected - all the other extenders have them as such.

@tankerkiller125

This comment has been minimized.

Copy link
Contributor Author

tankerkiller125 commented Dec 9, 2019

@datitisev I've fixed that now, don't know how I did that considering I was using Frontend as a template, must have been a lapse in thought.

@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Dec 12, 2019

I would rather see all those properties private for all extenders. Most of them are so small; if you want to change something about them, just create a new class. You can't trust us not to change e.g. the variable names anyway.

Anyway, let's merge for now.

@franzliedke franzliedke merged commit aba291c into flarum:master Dec 12, 2019
10 checks passed
10 checks passed
PHP 7.1 / MySQL
Details
PHP 7.1 / MariaDB
Details
PHP 7.2 / MySQL
Details
PHP 7.2 / MariaDB
Details
PHP 7.3 / MySQL
Details
PHP 7.3 / MySQL (prefix)
Details
PHP 7.3 / MariaDB
Details
PHP 7.3 / MariaDB (prefix)
Details
WIP Ready for review
Details
continuous-integration/styleci/pr The analysis has passed
Details
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Dec 12, 2019

Possible next steps:

  • More use-cases (removing, adding at different positions, replacing) - #1971
  • Extender tests (let's pair on those when it's my turn to coach)
@tankerkiller125 tankerkiller125 deleted the tankerkiller125:mk/middlware-extender branch Dec 12, 2019
@tankerkiller125 tankerkiller125 mentioned this pull request Dec 20, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.