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

Add a current user service #14

Merged
merged 10 commits into from
Dec 3, 2014
Merged

Add a current user service #14

merged 10 commits into from
Dec 3, 2014

Conversation

dawehner
Copy link
Collaborator

@dawehner dawehner commented Dec 2, 2014

A lot of code is getting the current user to get the uid or simple do some user_access().

Let's implement the 'current_user' service from D8

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8ca6477 on dawehner:current_user into e950cc9 on LionsAd:7.x-1.x.

drupal_save_session(FALSE);
$GLOBALS['user'] = $admin_user;

$this->assertEqual(array_keys($admin_user->roles), $account->getRoles());
Copy link
Owner

Choose a reason for hiding this comment

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

Need to re-get $account from the container before that.

Copy link
Owner

Choose a reason for hiding this comment

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

Or better yet check that old $account did not change.

@LionsAd
Copy link
Owner

LionsAd commented Dec 2, 2014

These failures are a consequence of a drupal_ti bug fixed in the add-behat-integration branch.

I will pull it tomorrow morning.

@LionsAd
Copy link
Owner

LionsAd commented Dec 2, 2014

drupal_ti is fixed.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 328abda on dawehner:current_user into e950cc9 on LionsAd:7.x-1.x.

parent::setUp($modules);

\ServiceContainer::init();
$this->container = \Drupal::getContainer();
Copy link
Owner

Choose a reason for hiding this comment

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

nit - Need to document the $this->container variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, it almost seems to be that we collect a bunch of issues before we can merge them.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9ec4eac on dawehner:current_user into e950cc9 on LionsAd:7.x-1.x.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9ec4eac on dawehner:current_user into e950cc9 on LionsAd:7.x-1.x.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9ec4eac on dawehner:current_user into e950cc9 on LionsAd:7.x-1.x.

// @see _drupal_session_write()
$sid = session_id();
if ($this->variable->get('https', FALSE)) {
$insecure_session_name = substr(session_name(), 1);
Copy link
Owner

Choose a reason for hiding this comment

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

This is the wrong way round and gets the insecure session ID from the secure one.

I would prefer if we did throw a BadMethodCallException for now.

It might be that mixed mode SSL is even removed from core.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should not used by application code anyway. Agreed.

@LionsAd
Copy link
Owner

LionsAd commented Dec 3, 2014

Last issue to fix, then RTBM.

* {@inheritdoc}
*/
public function getPreferredAdminLangcode($fallback_to_default = TRUE) {
throw new \Exception(sprintf('%s is not implemented', __FUNCTION__));
Copy link
Owner

Choose a reason for hiding this comment

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

BadMethodCallException?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right

@LionsAd
Copy link
Owner

LionsAd commented Dec 3, 2014

This needs a rebase, then RTBM.

@LionsAd LionsAd added RTBM and removed Needs review labels Dec 3, 2014
@dawehner
Copy link
Collaborator Author

dawehner commented Dec 3, 2014

Beside the rebase I decided to use the new base class in the tests.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5c3d757 on dawehner:current_user into 80ca2d6 on LionsAd:7.x-1.x.

LionsAd added a commit that referenced this pull request Dec 3, 2014
@LionsAd LionsAd merged commit 5a216f1 into LionsAd:7.x-1.x Dec 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants