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

[ticket/10899] Using Delete All in log viewer with keyword search #2433

Merged
merged 11 commits into from
May 30, 2014

Conversation

Nicofuma
Copy link
Member

@nickvergessen
Copy link
Contributor

As per IRC the log class should perform the deleting

@nickvergessen
Copy link
Contributor

Code looks good, but tests for log::delete() should be added

@@ -26,7 +26,7 @@ function main($id, $mode)
{
global $db, $user, $auth, $template, $cache, $phpbb_container;
global $config, $phpbb_root_path, $phpbb_admin_path, $phpEx;
global $request;
global $request, $phpbb_log;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get from container?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@bantu
Copy link
Collaborator

bantu commented May 27, 2014

Code looks good, but tests for log::delete() should be added

I agree

}

$sql = 'DELETE FROM ' . LOG_TABLE . "
$sql_where";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabs are off?
It should be "the same as before $sql" + 1

$log_type = false;
}

if ($log_type === false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an event for extensions before this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core.log_delete_before?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but see core.add_log and add a similar NOTE about the log_type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@nickvergessen
Copy link
Contributor

phpbb_event_export_php_test::test_crawl_php_file with data set #178 ('phpbb/log/log.php')
LogicException: Event name does not match '@event' tag for event 'core.delete_log' in file 'phpbb/log/log.php:385'

* NOTE: if sql_ary does not contain a log_type value, the entry will
* not be deleted in the database. So ensure to set it, if needed.
*
* @event core.add_log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*core.delete_log

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@nickvergessen nickvergessen merged commit 4b3bba6 into phpbb:develop-ascraeus May 30, 2014
nickvergessen added a commit that referenced this pull request May 30, 2014
[ticket/10899] Using Delete All in log viewer with keyword search

* Nicofuma/ticket/10899:
  [ticket/10899] Update doc block
  [ticket/10899] Use isset($field_value['IN'])
  [ticket/10899] Add event core.delete_log
  [ticket/10899] Remove trailing ;
  [ticket/10899] Fix typo in the class name
  [ticket/10899] Add unit tests
  [ticket/10899] Get $phpbb_log from the container
  [ticket/10899] Remove extra ';'
  [ticket/10899] Typo
  [ticket/10899] Refactoring in \phpbb\log\log_interface
  [ticket/10899] Using Delete All in log viewer with keyword search
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