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

Cleanup middleware registering #12621

Merged
merged 1 commit into from
Jan 7, 2019

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Nov 23, 2018

Fixes #12224

Since we only use the middleware at 1 location it makes no sense to
register them in each and every container.

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

@rullzer rullzer added this to the Nextcloud 16 milestone Nov 23, 2018
@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 8, 2018
@rullzer rullzer force-pushed the td/12224/cleanup_middleware_registering branch from bfbc52c to 30af35d Compare December 10, 2018 20:46
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good in general. Left two comments for code that might not be necessary anymore and can be cleaned up while touching it :)

@rullzer rullzer force-pushed the td/12224/cleanup_middleware_registering branch from 30af35d to 499668b Compare December 27, 2018 09:11
@MorrisJobke
Copy link
Member

@rullzer what is the status of this one?

@rullzer
Copy link
Member Author

rullzer commented Jan 3, 2019

@rullzer what is the status of this one?

status; waiting for me to have some time and finish it

@rullzer rullzer force-pushed the td/12224/cleanup_middleware_registering branch from 499668b to 4b695a4 Compare January 3, 2019 07:30
Fixes #12224

Since we only use the middleware at 1 location it makes no sense to
register them in each and every container.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the td/12224/cleanup_middleware_registering branch from 4b695a4 to 54ff913 Compare January 3, 2019 10:50
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good and works fine 👍

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good!

)
);
$dispatcher->registerMiddleware(
new CORSMiddleware(
Copy link
Member

Choose a reason for hiding this comment

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

In a follow-up PR we could also have these instances constructed by the container if possible 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we could. But it is less of an issue since it is a static list. But agreed for later.

@rullzer rullzer merged commit 8b0f5e0 into master Jan 7, 2019
@ChristophWurst ChristophWurst deleted the td/12224/cleanup_middleware_registering branch January 7, 2019 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants