Fix users do not have an associated email address

[alihamidali]

fix users do not have an associated email address

WHY

BEFORE - What was wrong? What was happening before this PR?

• You could not run php artisan route:list If you use different column instead of backpack default column for storing user emails.
• You could not use backpack_users_have_email function properly as it will always return false

AFTER - What is happening after this PR?

• backpack_users_have_email returns the right flag now
• php artisan route:list returns the list of all the routes

HOW

How did you achieve that, in technical terms?

replaced email by backpack_authentication_column function inside method backpack_users_have_email

Is it a breaking change or non-breaking change?

non-breaking

How can we test the before & after?

Try changing the authentication_column value in base.php e.g, username and then run php artisan r:l command from terminal.

[welcome[bot]]

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

• if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
• even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
• not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
• we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!

-- Justin Case The Backpack Robot

[pxpm]

@tabacitu I think I agree with this. But we should take it carefully.

This function is used to specifically check if users have email to enable/disable password reset functionality. https://github.com/Laravel-Backpack/CRUD/blob/057588417a9ae5ed780329de76cc9de406d58072/src/app/Http/Controllers/Auth/ResetPasswordController.php#L49

Also in other files like https://github.com/Laravel-Backpack/CRUD/blob/057588417a9ae5ed780329de76cc9de406d58072/src/resources/views/base/auth/register.blade.php#L78

This is not an easy task. At the moment this is technical debt in Backpack.

• You cannot use any other column name as "email", eg: user_mail because Backpack checks in code that the column name is email (the name) to be considered an email column. https://github.com/Laravel-Backpack/CRUD/blob/057588417a9ae5ed780329de76cc9de406d58072/src/resources/views/base/auth/register.blade.php#L30

The best bet when not using emails to authenticate is to completely disable the backpack authentication in config, that at the moment is the best solution.

I would like to discuss first with @tabacitu the idea of adding api authentication too to backpack, only after deciding what to do, mess with this bit of code (authentication). But indeed, it needs to be worked sooner than later.

Thanks for taking time to submit the PR @alihamidali 🙏

[tabacitu]

Hey,

Thanks for the PR @alihamidali 🙏

Great catch @pxpm ! This would make backpack_users_have_email() always return true, which would be breaking. I think we can make this non-breaking by adding a config value for the email, say:

// after
'backpack.base.authentication_column' => 'email',
// we add
'backpack.base.email_column' => 'email',

in config/backpack/base.php. It would be a bit weird since they'll both say email most of the time, but it'd at least fix the problem at hand, and give devs a way to customize the name of the email column.

Seems like what this PR needs in order to be non-breaking... right?

---

I would like to discuss first with @tabacitu the idea of adding api authentication too to backpack

Sounds like a separate issue altogether, to me.

[pxpm]

I agree, it's separate issue. I was just thinking fixing this while/if we work a little bit on authentication. I thought about the two configs, but I wouldn't have guessed that you wouldn't mind having such similar and confusing configs.

But indeed it would make it non break and fix the current problem. So.. if you agree, would you @alihamidali like to finish this pr with @tabacitu inputs?

Thanks 🙏

[tabacitu]

I agree, it's separate issue. I was just thinking fixing this while/if we work a little bit on authentication. I thought about the two configs, but I wouldn't have guessed that you wouldn't mind having such similar and confusing configs.

😅 Fair enough. How about this, would this be better?

// after
'backpack.base.authentication_column' => 'email',
// we add
'backpack.base.email_authentication' => true,

That second one would be used in backpack_users_have_email():

• if config('backpack.base.email_authentication') is true, return true;
• if config('backpack.base.email_authentication') is false, return false;
• if config('backpack.base.email_authentication') is null, inexisting or anything else, do what's done now - see if the table has an email column (hardcoded);

That would be cleaner and solve the problem, right?

[alihamidali]

@tabacitu @pxpm

I was wondering to ask your thoughts about the current pr changes, wouldn't it resolve the issue? To use the email column keyword dynamically instead of hard code check.

old

return \Schema::hasColumn($user->getTable(), 'email');

new changes inside backpack_users_have_email

return \Schema::hasColumn($user->getTable(), backpack_authentication_column());

[tabacitu]

Hey @alihamidali ,

I don't think so, no - as it is now, it would break backpack_users_have_email(). That function should return true/false to answer that question. With the PR as it is now, an important scenario isn't covered:

• you change the authentication column to username, because you do NOT have emails;
• you expect backpack_users_have_email() to return false, because again, there are no emails;
• but with this PR it would still return true;

So... yeah, it wouldn't work.

[jcastroa87]

@tabacitu @pxpm i create a PR https://github.com/Laravel-Backpack/CRUD/pull/4623 to fix this, i take @tabacitu idea and create new config to set email column, and now it can be set in base.php

[pxpm]

Hello @alihamidali

Thanks for the contribution, and sorry we can't merge it. We are working on a solution in #4623 so I will be closing this one to avoid confusions.

Once again, thank you very much for the contribution, keep 'em coming 🙏

Cheers