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.

Sign up for GitHub

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

Jump to bottom

Fix TokensList::getPrevious which was not able to reach very first token #428

Merged
merged 1 commit into from
Mar 11, 2023
Merged

Fix TokensList::getPrevious which was not able to reach very first token #428

merged 1 commit into from
Mar 11, 2023

Conversation

Tithugues
Copy link
Contributor

This change aims to fix a bug in the TokensList::getPrivious method which was not able to reach the very first token. As you can see in the test case, the method returned null before returning the SELECT token.

If it was not a bug, but an expected behavior, please let me know.

Maybe such a bugfix should be backported to 5.7 as well, but I can't find a maintenance branch for this version.

Thank you.

@Tithugues Tithugues mentioned this pull request Mar 8, 2023
Merged
williamdes
williamdes approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@williamdes williamdes left a comment •
edited

Choose a reason for hiding this comment

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

Do you have a SQL statement that could create a regression test for us ?

Maybe such a bugfix should be backported to 5.7 as well, but I can't find a maintenance branch for this version.

That's okay, 5.8 will replace 5.7 without any breaking changes. Like previous versions :)

@williamdes williamdes added this to the 5.8.0 milestone Mar 9, 2023
@Tithugues
Copy link
Contributor Author

Do you have a SQL statement that could create a regression test for us ?

I think that there already is one. The one used in the test class starts by SELECT, but as we can see in the test itself, when we are on index 2 (the * of the SELECT), then getting previous token again lead to null, whereas it should have lead to the SELECT token.

Is it clear enough? Does it make sense to you?

That's okay, 5.8 will replace 5.7 without any breaking changes. Like previous versions :)

Thanks! I hope it will be released soon then. 🙏

@williamdes williamdes self-assigned this Mar 11, 2023
williamdes added a commit that referenced this pull request Mar 11, 2023
@williamdes
Merge #428 - Fix TokensList::getPrevious which was not able to reach …
b5c995a 
…very first token

Pull-request: #428

Signed-off-by: William Desportes <williamdes@wdes.fr>
williamdes added a commit that referenced this pull request Mar 11, 2023
@williamdes
Add a CHANGELOG entry for #428
e2c0e89 
Pull-request: #428

Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes williamdes merged commit aa8a915 into phpmyadmin:5.8.x Mar 11, 2023
@williamdes williamdes added the bug label Mar 11, 2023
williamdes added a commit that referenced this pull request Mar 11, 2023
@williamdes
Fix coding standard issues after #428
a6efc9b 
Signed-off-by: William Desportes <williamdes@wdes.fr>
@Tithugues Tithugues deleted the fix/getPrevious branch March 11, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamdes williamdes williamdes approved these changes

Assignees

@williamdes williamdes

Labels
bug
Projects
None yet
Milestone
5.8.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Tithugues @williamdes