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

Fix _fromTables() #2174

Merged
merged 3 commits into from
Sep 23, 2019
Merged

Fix _fromTables() #2174

merged 3 commits into from
Sep 23, 2019

Conversation

pjsde
Copy link
Contributor

@pjsde pjsde commented Aug 30, 2019

When there are JOINs and more than one table exists, the FROM clause must be enclosed in parentheses, otherwise an error occurs if a JOIN does not refer to the last table.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

When there are JOINs and more than one table exists, the FROM clause must be enclosed in parentheses, otherwise an error occurs if a JOIN does not refer to the last table.
@pjsde pjsde changed the title Resolve BUG in _fromTables() Fix _fromTables() Aug 31, 2019
@jim-parry
Copy link
Contributor

@lonnieezell This looks good to me, and nothing seems missing. Should this be merged?

@lonnieezell
Copy link
Member

@pjsde A couple of quick questions:

  1. Can you confirm this syntax works for all database engines we currently support. It will have to if it's in BaseBuilder.
  2. Can you point us to existing tests that cover this situation? If not, we will need additional tests written to prove it works on all current engines.

@pjsde
Copy link
Contributor Author

pjsde commented Sep 19, 2019

@lonnieezell You are right, this change should not be in BaseBuilder, but in the database engines, as it is not valid on all database engines, being only valid for MySQL.

@jim-parry jim-parry merged commit 3a0133a into codeigniter4:develop Sep 23, 2019
@pjsde
Copy link
Contributor Author

pjsde commented Oct 4, 2019

Can anyone explain to me why despite the merge is done, the changes are not reflected in the develop branch, because if we consult the history of the file https://github.com/codeigniter4/CodeIgniter4/commits/develop/system/Database/MySQLi/Builder.php we can see that the last commit was made in May 4, 2019 by @jim-parry

@jim-parry
Copy link
Contributor

This is a mystery to me.
@lonnieezell Any ideas?

@lonnieezell
Copy link
Member

I have no idea. There was a change to MySQLi/Connection 17 days ago, but everything else was 5 months ago.

I guess make a new PR? This part seems out of our control and in GitHub's hands.

@jim-parry
Copy link
Contributor

I will check some of the other commits around the same time and see what else might have gone astray.

@pjsde
Copy link
Contributor Author

pjsde commented Oct 4, 2019

Ok. I'll make a new PR then.

@jim-parry
Copy link
Contributor

The commits before & after this one check out, i.e. the commits look right and the changes are in effect.
This commit looks right in the history, but the changes are not in effect.
The only thing I can attribute this to is a hiccup on github's end, possibly synching or replicating something in their database(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants