Skip to content

Fix #17635 - Semicolon repeatedly suffixed onto SQL if query ends in comment#18264

Open
LakshayaTohan wants to merge 1 commit intophpmyadmin:masterfrom
LakshayaTohan:fix-issue-17635-semicolon-repeatedly-suffixed
Open

Fix #17635 - Semicolon repeatedly suffixed onto SQL if query ends in comment#18264
LakshayaTohan wants to merge 1 commit intophpmyadmin:masterfrom
LakshayaTohan:fix-issue-17635-semicolon-repeatedly-suffixed

Conversation

@LakshayaTohan
Copy link
Copy Markdown

@LakshayaTohan LakshayaTohan commented Mar 16, 2023

Description

Added check for addition of semicolon only when it is missing while editing and/or running an SQL query.

Fixes #17635

Before submitting pull request, please review the following checklist:

  • Make sure you have read our CONTRIBUTING.md document.
  • Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
  • Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
  • Every commit has a descriptive commit message.
  • Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
  • Any new functionality is covered by tests.

@williamdes williamdes changed the title Fix for ChrisHSandN/phpmyadmin#17635 Fix #17635 - Semicolon repeatedly suffixed onto SQL if query ends in comment Mar 16, 2023
@LakshayaTohan LakshayaTohan force-pushed the fix-issue-17635-semicolon-repeatedly-suffixed branch from 1e28bf1 to 0f47d83 Compare March 16, 2023 12:33
@williamdes williamdes force-pushed the fix-issue-17635-semicolon-repeatedly-suffixed branch from 0f47d83 to 9b30512 Compare May 6, 2023 21:23
@codecov
Copy link
Copy Markdown

codecov bot commented May 6, 2023 •
edited
Loading

Codecov Report

Patch coverage: 88.88% and project coverage change: +0.02 🎉

Comparison is base (3156c39) 54.95% compared to head (9b30512) 54.97%.

Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #18264      +/-   ##
============================================
+ Coverage     54.95%   54.97%   +0.02%     
- Complexity    16428    16434       +6     
============================================
  Files           631      631              
  Lines         64544    64550       +6     
============================================
+ Hits          35467    35487      +20     
+ Misses        29077    29063      -14
Flag Coverage Δ
dbase-extension 54.97% <88.88%> (+0.03%) ⬆️
recode-extension 54.92% <88.88%> (+<0.01%) ⬆️
unit-8.1-ubuntu-latest 54.92% <88.88%> (+<0.01%) ⬆️
unit-8.2-ubuntu-latest 54.92% <88.88%> (+<0.01%) ⬆️
unit-nightly-ubuntu-latest 54.92% <88.88%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
libraries/classes/Import.php 26.70% <88.88%> (+1.09%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@williamdes williamdes added this to the 6.0.0 milestone May 7, 2023
@williamdes williamdes requested a review from MauricioFauth May 7, 2023 09:21
@williamdes
Copy link
Copy Markdown
Member

williamdes commented Apr 27, 2024

@kamil-tekiela You did some refactoring to this code, how can this be merged without needing to use that very costy str_contains function ?

@kamil-tekiela
Copy link
Copy Markdown
Contributor

kamil-tekiela commented Apr 27, 2024

str_contains is not very costly. I guess this just needs a rebase.

@williamdes williamdes mentioned this pull request Jan 13, 2025
Closed
@LakshayaTohan @williamdes
Fix phpmyadmin#17635 - Added check for semicolon appending at end of …
e2bfbc5 
…SQL query

Signed-off-by: Lakshaya Tohan <lakshaya.tohan@gmail.com>
@williamdes williamdes force-pushed the fix-issue-17635-semicolon-repeatedly-suffixed branch from 9b30512 to e2bfbc5 Compare March 16, 2026 23:09
@williamdes
Copy link
Copy Markdown
Member

williamdes commented Mar 17, 2026

Hi @kamil-tekiela
Is this still needed ?
No tests seem to be unhappy, this is odd

@kamil-tekiela
Copy link
Copy Markdown
Contributor

kamil-tekiela commented Mar 17, 2026

I don't think this should be merged because the semicolon is still added in the wrong place. This is really a job for the sql-parser and it can already do it (although there is a bug), but ImportSql is not the only place where this method is called from, so this needs a bit more refactoring.

@williamdes
Copy link
Copy Markdown
Member

williamdes commented Mar 17, 2026

Okay, if we can have a regression test first that would be good. Because the current code seems fine

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

@MauricioFauth MauricioFauth Awaiting requested review from MauricioFauth

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Semicolon repeatedly suffixed onto SQL if query ends in comment

3 participants

@LakshayaTohan @williamdes @kamil-tekiela