Skip to content
This repository was archived by the owner on Aug 31, 2022. It is now read-only.

Remove the OgRouteContext provider. This is now built in to OG.#12

Merged
claudiu-cristea merged 2 commits into
8.x-1.xfrom
remove-context-provider
Oct 12, 2016
Merged

Remove the OgRouteContext provider. This is now built in to OG.#12
claudiu-cristea merged 2 commits into
8.x-1.xfrom
remove-context-provider

Conversation

@pfrenssen

Copy link
Copy Markdown
Contributor

A proposal is made to include a generic context provider in OG: Gizra/og#180

The context provider in OG Menu is now obsolete and might even conflict with the one from OG.

@claudiu-cristea claudiu-cristea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems a straight solution. I think the service should also be removed in og_menu.services.yml:

  og_menu.og_route_context:
    class: Drupal\og_menu\ContextProvider\OgRouteContext
    arguments: ['@current_route_match']
    tags:
      - { name: 'context_provider' }

BTW, before reviewing this I did a check also on Gizra/og#180. Very nice work, congratulations!

@claudiu-cristea claudiu-cristea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See the above comment.

@claudiu-cristea

Copy link
Copy Markdown
Contributor

I requested changes but the issue doesn't turn red. Seems that I don't have some permissions on this project?

@claudiu-cristea claudiu-cristea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved

@pfrenssen

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Very good point, I also removed the declaration of the route context in the yaml file.

I don't know how the new review system works on Github, it opens some kind of floating dialog which I dislike.

@claudiu-cristea claudiu-cristea merged commit 580a027 into 8.x-1.x Oct 12, 2016
@claudiu-cristea claudiu-cristea deleted the remove-context-provider branch October 12, 2016 09:48
@pfrenssen

Copy link
Copy Markdown
Contributor Author

In the meantime I got the hang of the new review system, and @claudiu-cristea has the right permissions!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants