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

sql_return_on_error removal (11162) #1030

Merged
merged 22 commits into from Dec 14, 2012

Conversation

Projects
None yet
4 participants
@p
Contributor

p commented Nov 2, 2012

http://tracker.phpbb.com/browse/PHPBB3-11162

See the ticket comments for what to do with this PR.

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Nov 11, 2012

Member

I have some changes here, but I have to go now. bantu/phpbb@phpbb:develop-olympus...bantu:ticket/11162

Member

bantu commented Nov 11, 2012

I have some changes here, but I have to go now. bantu/phpbb@phpbb:develop-olympus...bantu:ticket/11162

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Nov 11, 2012

Member

The tests are also not passing right now, because there is no unique key.

Member

bantu commented Nov 11, 2012

The tests are also not passing right now, because there is no unique key.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Nov 15, 2012

Contributor

They won't work in postgres regardless of unique key.

Contributor

p commented Nov 15, 2012

They won't work in postgres regardless of unique key.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 2, 2012

Contributor

How about this contraption.

However, when testing it the topic_watch merging does not seem to work:

  1. Create a topic
  2. Watch it
  3. Merge it with another topic
  4. Old watched topic disappears from ucp watched topic list, new topic does not appear.

I checked develop which does not have this patch and behavior is the same. This is on postgres.

Contributor

p commented Dec 2, 2012

How about this contraption.

However, when testing it the topic_watch merging does not seem to work:

  1. Create a topic
  2. Watch it
  3. Merge it with another topic
  4. Old watched topic disappears from ucp watched topic list, new topic does not appear.

I checked develop which does not have this patch and behavior is the same. This is on postgres.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 2, 2012

Contributor

Consider merging this and #988 into the same branch.

Contributor

p commented Dec 2, 2012

Consider merging this and #988 into the same branch.

@EXreaction

This comment has been minimized.

Show comment
Hide comment
@EXreaction

EXreaction Dec 4, 2012

Contributor

I think phpbb_update_rows_avoiding_duplicates would be better as a function in the dbal. $db->update_rows_avoiding_duplicates

Contributor

EXreaction commented Dec 4, 2012

I think phpbb_update_rows_avoiding_duplicates would be better as a function in the dbal. $db->update_rows_avoiding_duplicates

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 4, 2012

Contributor

It is actually specific to the two supported tables. A generic function would be quite a bit more complicated.

Contributor

p commented Dec 4, 2012

It is actually specific to the two supported tables. A generic function would be quite a bit more complicated.

@EXreaction

This comment has been minimized.

Show comment
Hide comment
@EXreaction

EXreaction Dec 4, 2012

Contributor

I see.

It does look like it should support a few other tables (forums_track, sessions_keys, topics_track, topics_posted, and zebra from what I saw).

Does topics_watch need anything special done to it besides this? This way it does not compare notify_status (although I am not exactly sure I understand what this is used for).

I think then that the function just may need a better name since it's dependent on the user_id, maybe phpbb_update_rows_avoiding_duplicates_per_user

Contributor

EXreaction commented Dec 4, 2012

I see.

It does look like it should support a few other tables (forums_track, sessions_keys, topics_track, topics_posted, and zebra from what I saw).

Does topics_watch need anything special done to it besides this? This way it does not compare notify_status (although I am not exactly sure I understand what this is used for).

I think then that the function just may need a better name since it's dependent on the user_id, maybe phpbb_update_rows_avoiding_duplicates_per_user

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 5, 2012

Contributor

Notify status indicates whether the user was notified about a new post in the topic.

0 takes precedence over 1 in updates.

Contributor

p commented Dec 5, 2012

Notify status indicates whether the user was notified about a new post in the topic.

0 takes precedence over 1 in updates.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 5, 2012

Contributor

Another couple hundred lines of code and we are golden again.

Contributor

p commented Dec 5, 2012

Another couple hundred lines of code and we are golden again.

@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Dec 5, 2012

Contributor

I don't like the file name. Maybe put it into dbtools or name the file like functions_database_helper, so we can put in more stuff not only "tricky updates"?

Contributor

nickvergessen commented Dec 5, 2012

I don't like the file name. Maybe put it into dbtools or name the file like functions_database_helper, so we can put in more stuff not only "tricky updates"?

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Dec 5, 2012

Member

This is application specific code now and thus does not belong into dbal or dbtools.

Member

bantu commented Dec 5, 2012

This is application specific code now and thus does not belong into dbal or dbtools.

@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Dec 5, 2012

Contributor

So then my vote goes for " or name the file like functions_database_helper, so we can put in more stuff not only "tricky updates""

Contributor

nickvergessen commented Dec 5, 2012

So then my vote goes for " or name the file like functions_database_helper, so we can put in more stuff not only "tricky updates""

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 5, 2012

Contributor

Decide on the file name you want and I'll rename the file.

Contributor

p commented Dec 5, 2012

Decide on the file name you want and I'll rename the file.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 5, 2012

Contributor

Other comments fixed.

Contributor

p commented Dec 5, 2012

Other comments fixed.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 13, 2012

Contributor

Changed to database helper.

Contributor

p commented Dec 13, 2012

Changed to database helper.

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Dec 13, 2012

Member

Looks like excellent work to me.

Member

bantu commented Dec 13, 2012

Looks like excellent work to me.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 13, 2012

Contributor

Fixed agai.n

Contributor

p commented Dec 13, 2012

Fixed agai.n

@bantu bantu merged commit a686910 into phpbb:develop-olympus Dec 14, 2012

1 check passed

default The Travis build passed
Details
@EXreaction

This comment has been minimized.

Show comment
Hide comment
@EXreaction

EXreaction Dec 14, 2012

Contributor

I just noticed this now, but could we move tests/functions_database_helper/ to tests/functions/?

We already have tons of directories in tests.

Contributor

EXreaction commented Dec 14, 2012

I just noticed this now, but could we move tests/functions_database_helper/ to tests/functions/?

We already have tons of directories in tests.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 14, 2012

Contributor

Technically, we probably could.

Note that there are other functions_* subdirs there and they may differ between branches.

I am in slight favor of trying a single functions directory with everything.

Contributor

p commented Dec 14, 2012

Technically, we probably could.

Note that there are other functions_* subdirs there and they may differ between branches.

I am in slight favor of trying a single functions directory with everything.

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