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

Port OG-Context module as OgContext provider#180

Merged
amitaibu merged 112 commits into
Gizra:8.x-1.xfrom
amitaibu:242-simplified
Nov 8, 2016
Merged

Port OG-Context module as OgContext provider#180
amitaibu merged 112 commits into
Gizra:8.x-1.xfrom
amitaibu:242-simplified

Conversation

@pfrenssen

Copy link
Copy Markdown
Contributor

This is a continuation of amitaibu#252. It is a simplified version of the initial implementation of OgContext that was created in that PR.

@pfrenssen

Copy link
Copy Markdown
Contributor Author

This still needs a test for OgResolvedGroupCollection and for OgContext itself. OgResolvedGroupCollectionTest will be a very straightforward unit test, but I'm not yet sure what would be the best approach to test OgContext.

I feel hesitant to write a functional test for this, since we would have to provide a test module that contains "something" (e.g. a block or a page controller) that consumes the available context and outputs data about it that we can check. For me providing a test module specifically for this seems overkill. Also I have more fun things to do in my spare time than waiting for functional tests to run 😁

A unit test will be quite complicated since much of the business logic in in protected methods, so this would mean that we need to do a ton of mocking.

Maybe a kernel test would be a solution, but this functionality relies on requests being made, which is normally in the domain of functional tests. I don't know how to 'fake' a request in a kernel test.

I will investigate this further in the weekend. For the moment I'm pretty happy with how this is turning out. It also seems to work quite nice in practice. I just replaced our project's custom context provider with this and ran our entire Behat test suite against it and it came back green.

@amitaibu amitaibu changed the title WIP: Simplified version of the OgGroupResolver based OgContext provider Port OG-Context module as OgContext provider Oct 26, 2016
@amitaibu

Copy link
Copy Markdown
Member

This will require a re-roll now.

@pfrenssen

Copy link
Copy Markdown
Contributor Author

Thanks for letting me know. Going to pick this back up!

@pfrenssen

Copy link
Copy Markdown
Contributor Author

Haha thanks for the compliment! :) I'm having great fun writing these, it's like a bunch of little puzzles to solve, and I'm learning so much about the internal workings of things like typed data.

This test is not very clean, but this is due to ContextProviders not using dependency injection. They use typed data which they get from \Drupal::typedDataManager() so I have to put a mocked TypedDataManager on the container.

I'm currently sprinting at a local open source conference so there is a good chance I will be able to finally finish this today. Fingers crossed :)

@pfrenssen

Copy link
Copy Markdown
Contributor Author

Alright that went pretty smooth! I think this is ready!

@amitaibu care for a look?

@amitaibu

amitaibu commented Nov 7, 2016

Copy link
Copy Markdown
Member

Yay! (Hectic days so might take a couple of days for me to have time to do proper review.)

@amitaibu

amitaibu commented Nov 8, 2016

Copy link
Copy Markdown
Member

I saw two todos there, but there's really no reason to not get this in. Thank you!!

@amitaibu amitaibu merged commit dbffd5d into Gizra:8.x-1.x Nov 8, 2016
@amitaibu amitaibu deleted the 242-simplified branch November 8, 2016 18:11
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.

3 participants