Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Allow column names to contain any character #249

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

metalinspired
Copy link

In regards to issues #8 and #208 I've created a regex that will allow user to use any character as column name.
It will also keep functionality of allowing user to enter 'column as alias' as column name.
I've made an assumption that no one in right mind will have spaces on beginning or end of column name so they are ignored by regex.

In regards to issues zendframework#8 and zendframework#208 I've created a regex that will allow user to use any character as column name.
It will also keep functionality of allowing user to enter 'column as alias' as column name.
I've made an assumption that no one in right mind will have spaces on beginning or end of column name so they are ignored by regex.
@ezimuel
Copy link
Contributor

ezimuel commented Jun 21, 2017

@metalinspired unit tests are missing for this PR, can you add it? Testing is very important here because you proposed a regex and we need to validate all the possible cases (or at least the most common). Thanks!

@metalinspired
Copy link
Author

@ezimuel I wouldn't know where to begin. The regex if written so that all existing tests are passing and as you can see I failed three times before succeeding. If the regex fails it will throw an exception and worse that can happen is malformed SQL syntax which was original problem to begin with. Personally I think that some of the undocumented behavior for column names, which I mentioned in #208, should be removed thus eliminating need for this patch.

@ezimuel
Copy link
Contributor

ezimuel commented Jun 21, 2017

@metalinspired I'll try to work on this PR adding the unit test, thanks anyway for your contribution!

@metalinspired
Copy link
Author

@ezimuel Glad I could help

@binal-7span
Copy link

Hey @froschdesign @ezimuel - May we have any update for this? We need this in one of our products. So it'll be a great help if this PR gets merged.

@michalbundyra
Copy link
Member

@BJGajjar Still we need unit test for these changes. If you can provide these it will speed up the release. Thanks.

@binal-7span
Copy link

binal-7span commented Oct 9, 2019

@metalinspired - It seems like there is some issue here.

  • When tried to add a field named with @!#!12123123; DB will have a column with the name of @`!`#`!`12123123 instead of @!#!12123123
  • When adding any field with special character[@user_id] - Although it's getting stored in DB - It is throwing Unable to find field @user_id error with status code 404 instead of the success code with the JSON object of stored field.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#70.

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

Successfully merging this pull request may close these issues.

6 participants