Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

#62489: dba_insert not working as expected #125

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

removed warnings if dba_insert failed because of an already existing key.

fixed handlers are:

  • flatfile
  • gdbm
  • inifile
  • qdbm
Contributor

smalyshev commented Jul 15, 2012

Please add a test.

@smalyshev: Tests added now

The test for the inifile handler fails because this handler insert items even if the item already exists.
I removed the waring "Key already exists" within DBA_UPDATE_FUNC(inifile) but the switch case never occur because inifile_append doesn't implement such functionality.

I'm not a C developer so it would be the best merging this but leaving the bug open so another person can fix it.

Hello guys, is there someone could review this ?

@lstrojny lstrojny commented on an outdated diff Jan 6, 2013

ext/dba/dba_gdbm.c
@@ -104,11 +104,16 @@
gval.dptr = (char *) val;
gval.dsize = vallen;
- if(gdbm_store(dba->dbf, gkey, gval,
- mode == 1 ? GDBM_INSERT : GDBM_REPLACE) == 0)
+ switch(gdbm_store(dba->dbf, gkey, gval, mode == 1 ? GDBM_INSERT : GDBM_REPLACE)) {
+ case -1:
@lstrojny

lstrojny Jan 6, 2013

Contributor

Indenting is wrong here.

@lstrojny lstrojny commented on an outdated diff Jan 6, 2013

ext/dba/dba_qdbm.c
return SUCCESS;
-
- php_error_docref2(NULL TSRMLS_CC, key, val, E_WARNING, "%s", dperrmsg(dpecode));
+ }
+
+ if (dpecode != DP_EKEEP) php_error_docref2(NULL TSRMLS_CC, key, val, E_WARNING, "%s", dperrmsg(dpecode));
@lstrojny

lstrojny Jan 6, 2013

Contributor

Braces are missing here

@lstrojny lstrojny commented on an outdated diff Jan 6, 2013

ext/dba/dba_gdbm.c
@@ -104,11 +104,16 @@
gval.dptr = (char *) val;
gval.dsize = vallen;
- if(gdbm_store(dba->dbf, gkey, gval,
- mode == 1 ? GDBM_INSERT : GDBM_REPLACE) == 0)
+ switch(gdbm_store(dba->dbf, gkey, gval, mode == 1 ? GDBM_INSERT : GDBM_REPLACE)) {
@lstrojny

lstrojny Jan 6, 2013

Contributor

Space after switch (before () missing

Contributor

lstrojny commented Jan 6, 2013

@marc-mabe sorry for the long delay. Please fix the coding style issues after that it looks good for merge.

@lstrojny : thank you for review! Updated coding style issues now

@lstrojny lstrojny and 1 other commented on an outdated diff Jan 6, 2013

ext/dba/dba_gdbm.c
@@ -104,11 +104,16 @@
gval.dptr = (char *) val;
gval.dsize = vallen;
- if(gdbm_store(dba->dbf, gkey, gval,
- mode == 1 ? GDBM_INSERT : GDBM_REPLACE) == 0)
- return SUCCESS;
- php_error_docref2(NULL TSRMLS_CC, key, val, E_WARNING, "%s", gdbm_strerror(gdbm_errno));
- return FAILURE;
+ switch (gdbm_store(dba->dbf, gkey, gval, mode == 1 ? GDBM_INSERT : GDBM_REPLACE)) {
+ case -1:
+ php_error_docref2(NULL TSRMLS_CC, key, val, E_WARNING, "%s", gdbm_strerror(gdbm_errno));
+ return FAILURE;
+ default:
@lstrojny

lstrojny Jan 6, 2013

Contributor

Is this really correct? Shouldn’t be the default case just throw some error?

@marc-mabe

marc-mabe Jan 6, 2013

The default case should be impossible - it was copied from DBA_UPDATE_FUNC(flatfile)

  • What should the message say on this case ?
  • "Unknown return value %value% by %lib-function%" ?
  • error, warning or notice?
@lstrojny

lstrojny Jan 14, 2013

Contributor

"Unknown return value" is fine. Warning seems appropriate to me.

Contributor

lstrojny commented Jan 14, 2013

After fixing the switch case, could you rebase and squash?

@marc-mabe marc-mabe Bug #62489: Removed warning on dba_insert if item already exists (aff…
…ected handlers are flatfile, inifile, qdbm and gdbm)
4881994

done! - updated the switch statements and squashed all commits together

@lstrojny lstrojny commented on the diff Jan 15, 2013

ext/dba/dba_qdbm.c
@@ -96,13 +96,15 @@
DBA_UPDATE_FUNC(qdbm)
{
QDBM_DATA;
- int result;
-
- result = dpput(dba->dbf, key, keylen, val, vallen, mode == 1 ? DP_DKEEP : DP_DOVER);
- if (result)
+
@lstrojny

lstrojny Jan 15, 2013

Contributor

Trailing spaces

@lstrojny lstrojny commented on the diff Jan 15, 2013

ext/dba/dba_qdbm.c
return SUCCESS;
-
- php_error_docref2(NULL TSRMLS_CC, key, val, E_WARNING, "%s", dperrmsg(dpecode));
+ }
+
@lstrojny

lstrojny Jan 15, 2013

Contributor

Trailing spaces

@lstrojny lstrojny commented on the diff Jan 15, 2013

ext/dba/dba_qdbm.c
return SUCCESS;
-
- php_error_docref2(NULL TSRMLS_CC, key, val, E_WARNING, "%s", dperrmsg(dpecode));
+ }
+
+ if (dpecode != DP_EKEEP) {
+ php_error_docref2(NULL TSRMLS_CC, key, val, E_WARNING, "%s", dperrmsg(dpecode));
+ }
+
@lstrojny

lstrojny Jan 15, 2013

Contributor

Trailing spaces

Contributor

lstrojny commented Jan 15, 2013

Merged into 5.5 and master. Cannot close it now as GitHub seems to be in trouble.

Comment on behalf of lstrojny at php.net:

Merged

@php-pulls php-pulls closed this Jan 15, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment