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

Jump to bottom

GH-310: Adds select validation to create view #311

Merged
merged 1 commit into from Oct 2, 2020
Merged

GH-310: Adds select validation to create view #311

merged 1 commit into from Oct 2, 2020

Conversation

aszenz
Copy link
Contributor

@aszenz aszenz commented Aug 23, 2020

The view statement body was not being validated, used existing
select statement to ensure that the view definition is correctly
linted.
Also updated generated parser tests in / out tests.

Fixes #310

@codecov
Copy link

codecov bot commented Aug 23, 2020 •
edited

Codecov Report

Merging #311 into QA will decrease coverage by 0.02%.
The diff coverage is 94.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##                 QA     #311      +/-   ##
============================================
- Coverage     99.73%   99.71%   -0.03%     
- Complexity     1899     1906       +7     
============================================
  Files            63       63              
  Lines          4581     4593      +12     
============================================
+ Hits           4569     4580      +11     
- Misses           12       13       +1
Impacted Files Coverage Δ Complexity Δ
src/Statements/CreateStatement.php 99.56% <94.44%> (-0.44%) 81.00 <0.00> (+7.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0da85e...de56687. Read the comment docs.

williamdes
williamdes approved these changes Aug 23, 2020
View reviewed changes
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

LGTM 👏 💯

@williamdes williamdes added this to the 4.6.2 milestone Aug 23, 2020
devenbansod
devenbansod requested changes Aug 23, 2020
View reviewed changes
Copy link
Member

@devenbansod devenbansod left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @aszenz! 👍

Left a comment/suggestion. Also, please rebase the branch over latest phpmyadmin/master.

src/Statements/CreateStatement.php Outdated Show resolved Hide resolved
@aszenz aszenz force-pushed the fix-missing-linting-on-view-select-stmnts branch from cdbed17 to 17a2aeb Compare August 25, 2020 17:40
@aszenz aszenz changed the base branch from QA to master August 25, 2020 17:41
@aszenz aszenz force-pushed the fix-missing-linting-on-view-select-stmnts branch from 17a2aeb to 31e7c31 Compare August 25, 2020 17:48
@aszenz
Copy link
Contributor Author

aszenz commented Aug 25, 2020

Left a comment/suggestion.

Made some changes based on these

Also, please rebase the branch over latest phpmyadmin/master.

This branch was originally made from QA, I switched both the pr and branch to master but now phpstan is complains about existing code and new code in the travis-ci.
Should I fix the existing code as well in this branch?

devenbansod
devenbansod approved these changes Aug 26, 2020
View reviewed changes
Copy link
Member

@devenbansod devenbansod left a comment

Choose a reason for hiding this comment

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

@aszenz I'm sorry for that earlier suggestion to rebase over master. I think QA might be the way to go.
@williamdes could you help decide which branch to target for this?

If all the tests pass and coverage is good, I'm okay to merge it once we decide on the target branch.

@aszenz @williamdes
Validates select inside view
de56687 
The view statement body was not being validated, used existing
select statement to ensure that the view definition is correctly
linted.
Also updated generated parser tests in / out tests.
@williamdes williamdes force-pushed the fix-missing-linting-on-view-select-stmnts branch from 31e7c31 to de56687 Compare August 27, 2020 08:25
@williamdes williamdes changed the base branch from master to QA August 27, 2020 08:25
@williamdes
Copy link
Member

@aszenz I'm sorry for that earlier suggestion to rebase over master. I think QA might be the way to go.
@williamdes could you help decide which branch to target for this?

If all the tests pass and coverage is good, I'm okay to merge it once we decide on the target branch.

Rebased (downbased) onto QA

@williamdes williamdes closed this Aug 27, 2020
@williamdes williamdes reopened this Aug 27, 2020
@williamdes
Copy link
Member

williamdes commented Aug 27, 2020 •
edited

Waiting on https://travis-ci.org/github/phpmyadmin/sql-parser/builds/721614968 now :)

Passed 🚀

devenbansod
devenbansod approved these changes Aug 29, 2020
View reviewed changes
Copy link
Member

@devenbansod devenbansod left a comment

Choose a reason for hiding this comment

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

LGTM 👍 💯

@williamdes williamdes self-assigned this Aug 29, 2020
@williamdes williamdes merged commit dca5a97 into phpmyadmin:QA Oct 2, 2020
@williamdes
Copy link
Member

Thank you @aszenz for your contribution and patience !
This will be part of the next 4.7.0 and 5.4.0 releases

@shucon shucon mentioned this pull request Jan 10, 2021
Closed
williamdes
williamdes reviewed Aug 14, 2021
View reviewed changes
src/Statements/CreateStatement.php
$token = $list->tokens[$list->idx];
if ($token->type === Token::TYPE_DELIMITER) {
break;
// Parsing the SELECT expression with and without the `AS` keyword
Copy link
Member

Choose a reason for hiding this comment

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

Hi @aszenz
Do you have an example of a create view without an AS ?

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 MauricioFauth approved these changes

@devenbansod devenbansod devenbansod approved these changes

Assignees

@williamdes williamdes

Labels
None yet
Projects
None yet
Milestone
4.7.0
Development

Successfully merging this pull request may close these issues.

Linter accepts false syntax in view definitions
4 participants
@aszenz @williamdes @MauricioFauth @devenbansod