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

[DoctrineBridge] fix case sensitivity issue in RememberMe\DoctrineTokenProvider #27314

Merged
merged 1 commit into from Nov 26, 2018
Merged

Conversation

PF4Public
Copy link
Contributor

@PF4Public PF4Public commented May 19, 2018

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? ?
Fixed tickets #21467
License MIT
Doc PR -

@nicolas-grekas nicolas-grekas changed the title changed lastUsed to lowercase to address and fix #21467 [DoctrineBridge] changed lastUsed to lowercase to address and fix #21467 May 21, 2018
@nicolas-grekas nicolas-grekas added this to the 2.7 milestone May 21, 2018
@nicolas-grekas
Copy link
Member

Hi @PF4Public, thanks for you PR. Would you mind rebasing/retargetting for branch 2.7? 2.3 is not maintained anymore and 2.7 will go out of maintenance in 10 days btw)

@PF4Public PF4Public changed the base branch from 2.3 to 2.7 May 21, 2018 18:27
@PF4Public
Copy link
Contributor Author

Hi @nicolas-grekas
Looks like I did it, please check if I've done it corectly.

@nicolas-grekas
Copy link
Member

We now need to figure out how not to break existing apps.

@PF4Public
Copy link
Contributor Author

Should I rebase it against newest version, which allows BC breaks?

@nicolas-grekas
Copy link
Member

We always strive for creating a continuous upgrade path so that there is never a hard BC break. This means e.g. triggering a deprecation notice when applicable, etc.

@fabpot fabpot modified the milestones: 2.7, 2.8 May 28, 2018
@fabpot
Copy link
Member

fabpot commented Oct 10, 2018

How can we move forward here?

@PF4Public
Copy link
Contributor Author

PF4Public commented Oct 10, 2018

Since this change could in fact break existing installations, it probably needs to be done in two steps: with deprecation notice first.
Sadly, I simply do not know, how are such situations handled in Symfony.
Alternatively we can do isset($row['lastUsed']) ? $row['lastUsed'] : $row['lastused'], like @sstok suggested. But I'm not sure, which makes the best fix...

@nicolas-grekas nicolas-grekas changed the base branch from 2.7 to 2.8 November 24, 2018 17:05
@nicolas-grekas nicolas-grekas changed the title [DoctrineBridge] changed lastUsed to lowercase to address and fix #21467 [DoctrineBridge] fix case sensitivity issue in RememberMe\DoctrineTokenProvider Nov 24, 2018
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I pushed a new fix for this issue on your fork.
PR ready.

@PF4Public
Copy link
Contributor Author

@nicolas-grekas , Thank you for fixing this. You've found a really clever way!

@@ -50,15 +50,16 @@ public function __construct(Connection $conn)
*/
public function loadTokenBySeries($series)
{
$sql = 'SELECT class, username, value, lastUsed'
// the alias for lastUsed works around case insensitivity in PostgreSQL
$sql = 'SELECT class, username, value, lastUsed as last_used'
Copy link
Member

Choose a reason for hiding this comment

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

AS?

@fabpot
Copy link
Member

fabpot commented Nov 26, 2018

Thank you @PF4Public.

@fabpot fabpot merged commit 0248d4f into symfony:2.8 Nov 26, 2018
fabpot added a commit that referenced this pull request Nov 26, 2018
…DoctrineTokenProvider (PF4Public)

This PR was merged into the 2.8 branch.

Discussion
----------

[DoctrineBridge] fix case sensitivity issue in RememberMe\DoctrineTokenProvider

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | ?
| Fixed tickets | #21467
| License       | MIT
| Doc PR        | -

Commits
-------

0248d4f [DoctrineBridge] fix case sensitivity issue in RememberMe\DoctrineTokenProvider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants