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

Add ability to provide db_id to avp_db_query() as either $var or $avp #694

Closed
mishehu opened this issue Nov 9, 2015 · 8 comments
Closed
Assignees

Comments

@mishehu
Copy link

mishehu commented Nov 9, 2015

Spoke with Vlad-Paiu about this. Currently avp_db_query() requires that the db_id (3rd param) be a constant. By allowing for avp_db_query() to use a $var and $avp, it would allow simplified iteration across an arbitrary number of avpops db urls as defined by the user, and keep from unnecessarily duplicating code.

If it wouldn't be too much trouble, could this functionality please be added? Much thanks in advance.

@ionutrazvanionita ionutrazvanionita self-assigned this Nov 11, 2015
@ionutrazvanionita
Copy link
Contributor

@mishehu would you need both string and int values for the db id in a variable?
I mean it's enough to have something like

$var(id)=1;
avp_db_query(...., "$var(id)");

or you would also need something like

$var(id) = "1";
avp_db_query(...., "$var(id)");

@mishehu
Copy link
Author

mishehu commented Nov 12, 2015

@ionutrazvanionita - As far as how I'm going to use it, I will be using only an integer value. But to be honest, maybe other people will find it useful to accept a string as well? I'm not sure. Integer is fine for me, but I'll defer to your better judgment. :-)

@ionutrazvanionita
Copy link
Contributor

That's why i asked, because i didn't know how you will use it. Anyhow, i made it work both use. Please check if everything is ok and close the ticket if so 8a51273

@EcosystemDev
Copy link

This works great. Thank you for including this.

@mishehu
Copy link
Author

mishehu commented Nov 21, 2015

Yep, it's working great for us, thanks for making the change!

@mishehu mishehu closed this as completed Nov 21, 2015
@mishehu
Copy link
Author

mishehu commented Jan 18, 2016

@ionutrazvanionita : There seems to be a problem at the current time. We're using this patch on the 2.1 branch, and it doesn't look like it was backported to 2.1. Can you please verify? The patch you previously provided works for 4 out of 5 hunks. There is an avpops.c.rej that I'm attaching. This wa done against commit 0a642c5 . Thanks!
avpops.c.rej.txt

@mishehu mishehu reopened this Jan 18, 2016
@ionutrazvanionita
Copy link
Contributor

@mishehu done d0f5c3d

@mishehu mishehu closed this as completed Feb 26, 2016
@mishehu
Copy link
Author

mishehu commented Feb 26, 2016

Sorry, missed closing this out for you. :-)

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

No branches or pull requests

3 participants