Problem/Motivation

API documentation for \Drupal\Core\Session\AccountInterface::getEmail is incorrect. When the method is called from the anonymous user (User::load(0);, NULL value is returned

Proposed resolution

Change/update return type and description.

Remaining tasks

Update API docs.

User interface changes

Nil

API changes

Only docs.

Data model changes

Nil

CommentFileSizeAuthor
#26 2932774-25.patch602 bytesharsha012
#24 interdiff-2932774-16-24.txt493 bytesharsha012
#24 2932774-24.patch705 bytesharsha012
#18 2932774-16.patch705 bytesharsha012
#10 2932774-10.patch708 bytesgaurav.kapoor
#7 2932774-7.patch644 bytesneelam.chaudhary
#2 2932774-2.patch628 bytesgaurav.kapoor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpi created an issue. See original summary.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commented
Status: Active » Needs review
FileSize
2932774-2.patch628 bytes

Did the required changes.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -146,9 +146,10 @@ public function getAccountName();
+   * Returns the email address of this account.For anonymous user NULL value is

    The short comment should not wrap. One line only. 

  2. +++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -146,9 +146,10 @@ public function getAccountName();
    *   The email address.

    return comment should specify what happens when there is no address.

neelam.chaudhary’s picture

neelam.chaudhary CreditAttribution: neelam.chaudhary as a volunteer and at Publicis Sapient commented
Status: Needs work » Needs review
FileSize
2932774-4.patch644 bytes

Fixing #3 points.

mohit1604’s picture

mohit1604 CreditAttribution: mohit1604 as a volunteer commented
Status: Needs review » Needs work
mohit1604’s picture

mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commented
Status: Needs work » Reviewed & tested by the community

Thanks, neelam.chaudhary for the patch #4.

neelam.chaudhary’s picture

neelam.chaudhary CreditAttribution: neelam.chaudhary as a volunteer and at Publicis Sapient commented
FileSize
2932774-7.patch644 bytes

@Mohit Malik Please do not delete or hide the patch if its working fine.
Adding the patch again as Mohit has hide the patch.

mohit1604’s picture

mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commented

@neelam.chaudhary Thanks for the patch mam ! . Patch was hidden/delete by mistake due to some network issues !

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -146,10 +146,10 @@ public function getAccountName();
+   *   The email address, or NULL if the account is anonymous.

The email field is now optional too, so this can return NULL for more than the anonymous users. Can we update this to reflect that? Possibly The email address, or NULL if the account is anonymous or the user does not have an email address

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
2932774-10.patch708 bytes

Did changes as per suggestion in #10.

jeetendrakumar’s picture

jeetendrakumar CreditAttribution: jeetendrakumar as a volunteer and at HyTechPro.com commented
Status: Needs review » Reviewed & tested by the community
jeetendrakumar’s picture

jeetendrakumar CreditAttribution: jeetendrakumar as a volunteer and at HyTechPro.com commented

Patch is working find for me
Changing status to RTBC

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -146,10 +146,11 @@ public function getAccountName();
+   * Returns the email address of this account if exists.

Can we make this 'if it exists'

Sorry for being pedantic.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Do we even need to change that line when we have the explicit documentation on the @return? I'd vote to just leave that initial line unchanged, seems enough to have that below?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Do we even need to change that line when we have the explicit documentation on the @return? I'd vote to just leave that initial line unchanged, seems enough to have that below?

Agree, that is much simpler

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
2932774-16.patch608 bytes

Adding a patch based on suggestions in #14.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Yes, that is enough I think.

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Reviewed & tested by the community » Needs review
FileSize
2932774-16.patch705 bytes

fixed nit pick

jeetendrakumar’s picture

jeetendrakumar CreditAttribution: jeetendrakumar as a volunteer and at HyTechPro.com commented
Status: Needs review » Reviewed & tested by the community

@Berdir

I think #18 patch have right documentation.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commented

@harsha012
This change isn't required

-   * Returns the email address of this account.
+   * Returns the email address of this account if it exists.

And which nitpick did you fix in #18?

The last submitted patch, , failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Patch #16 is the correct one

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs work

Adding review credit for

+++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -148,8 +148,9 @@ public function getDisplayName();
+   * @return string|NULL

this needs to be string|null, not string|NULL, for coding standards.

For whoever rerolls this, please use patch 16, not 18

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2932774-24.patch705 bytes
interdiff-2932774-16-24.txt493 bytes

please see comment #26

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
FileSize
2932774-25.patch602 bytes

fixed the nit picks as per #23

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commented
Status: Needs review » Reviewed & tested by the community

Since all the concerns have been addressed changing to RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Removing myself from issue credits

  • larowlan committed 354462c on 8.5.x 
    Issue #2932774 by harsha012, gaurav.kapoor, neelam.chaudhary, dpi:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.5.x-dev » 8.4.x-dev

Committed as 354462c and pushed to 8.5.x

Leaving at RTBC and changing to 8.4.x.

Will cherry-pick to 8.4.x tomorrow after commit freeze for 8.4.4 is over.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue tags: +8.4.4 Commit freeze

inventing a tag for myself

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2932774-25.patch, failed testing. View results

  • larowlan committed 4364f36 on 8.4.x 
    Issue #2932774 by harsha012, gaurav.kapoor, neelam.chaudhary, dpi:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs work » Fixed
Issue tags: -8.4.4 Commit freeze

Cherry-picked as 4364f36 and pushed to 8.4.x.

  • larowlan committed be8d160 on 8.4.x 
    Revert "Issue #2932774 by harsha012, gaurav.kapoor, neelam.chaudhary,...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.4.x-dev » 8.5.x-dev

As per https://www.drupal.org/core/release-cycle-overview#current-development-c... 8.4 is in 'critical fixes only', so I reverted this from 8.4

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.