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/12606] Add ACP group core events #2518

Merged
merged 1 commit into from Jun 9, 2014

Conversation

@Pico
Copy link
Contributor

commented May 30, 2014

'group_id',
'group_row',
'error',
'template_data',

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 30, 2014

Contributor

template_data should not be required. You can simply use template->assign_vars() yourself.

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 30, 2014

Contributor

Add group_desc_data, group_name, group_type, group_rank, rank_options

/**
* Initialise variables test before we display the add/edit form
*
* @event core.acp_group_options_variables_test

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 30, 2014

Contributor

event name should not focus on variables_test as it can be used for any data changes before saving them

* @var array validation_checks Array with validation data
* @since 3.1.0-b4
*/
$vars = array('action', 'submit_ary', 'validation_checks');

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 30, 2014

Contributor

please include group_id, group_row, error, group_name, group_desc, allow_desc_bbcode, allow_desc_urls, allow_desc_smilies and group_type

* @var array test_variables Array with variables for test
* @since 3.1.0-b4
*/
$vars = array('test_variables');

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 30, 2014

Contributor

Add action, submit_ary, group_id, group_row, error, group_name, group_desc, allow_desc_bbcode, allow_desc_urls, allow_desc_smilies and group_type

@Pico

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2014

Updated. Now I think if it require 3 events or just use 2 (remove the first one).

'group_rank',
'rank_options',
'error',
);

This comment has been minimized.

Copy link
@ghost

ghost May 30, 2014

Missing template_data? The other two events have their arrays included.

This comment has been minimized.

Copy link
@nickvergessen

This comment has been minimized.

Copy link
@ghost

ghost May 30, 2014

Ah, I missed that somehow. Thanks!

@Pico

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2014

Updated and rebased.

'allow_desc_urls',
'allow_desc_smilies',
'submit_ary',
'test_variables'

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 30, 2014

Contributor

Missing , at end of line:

There was 1 error:

  1. phpbb_event_export_php_test::test_crawl_php_file with data set #112 ('includes/acp/acp_groups.php')
    LogicException: Found invalid var 'test_variables, 'submit_ary' in array for event 'core.acp_manage_group_initialise_data' in file 'includes/acp/acp_groups.php:514'
* @var bool allow_desc_smilies Allow smiles in group description: true|false
* @var array submit_ary Array with new group data
* @var array validation_checks Array with validation data
* @since 3.1.0-b4

This comment has been minimized.

Copy link
@Pico

Pico May 30, 2014

Author Contributor

B4 was released.
So B5 or RC1?

This comment has been minimized.

Copy link
@nickvergessen
/**
* Request group data and operate on it
*
* @event core.acp_group_manage_request_data

This comment has been minimized.

Copy link
@ghost

ghost May 31, 2014

Maybe this is just being picky, but for consistent naming, should this be manage_group instead of group_manage?

This comment has been minimized.

Copy link
@Pico

Pico May 31, 2014

Author Contributor

No problem. I will change it.

* S_ERROR and ERROR_MSG to display it
* @var string group_name The group name
* @var string group_desc The group description
* @var string group_type The group type

This comment has been minimized.

Copy link
@prototech

prototech Jun 5, 2014

Contributor

This is an integer.

* S_ERROR and ERROR_MSG to display it
* @var string group_name The group name
* @var string group_desc The group description
* @var string group_type The group type

This comment has been minimized.

Copy link
@marc1706

marc1706 Jun 5, 2014

Member

see above comment by prototech

* @var int group_id The group id
* @var array group_row Array with new group data
* @var string group_name The group name
* @var string group_type The group type

This comment has been minimized.

Copy link
@marc1706

marc1706 Jun 5, 2014

Member

Should be int, too

* @event core.acp_manage_group_request_data
* @var string action Type of the action: add|edit
* @var int group_id The group id
* @var array group_row Array with new group data

This comment has been minimized.

Copy link
@prototech

prototech Jun 5, 2014

Contributor

Tabbing seems off in this line and the one below it.

* @event core.acp_manage_group_initialise_data
* @var string action Type of the action: add|edit
* @var int group_id The group id
* @var array group_row Array with new group data

This comment has been minimized.

Copy link
@prototech

prototech Jun 5, 2014

Contributor

Tabbing is off here, next line, and test_variables.

* S_ERROR and ERROR_MSG to display it
* @var string group_name The group name
* @var string group_desc The group description
* @var INT group_type The group type

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 9, 2014

Contributor

INT should be lowercase

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2014

Please either merge all the commits into one, or give them unique commit messages describing what each commit changed

@Pico

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2014

Updated and merged to 1 commit

nickvergessen added a commit to nickvergessen/phpbb that referenced this pull request Jun 9, 2014
Merge pull request phpbb#2518 from Pico88/ticket/12606
[ticket/12606] Add ACP group core events

* Pico88/ticket/12606:
  [ticket/12606] Add ACP group core events

@nickvergessen nickvergessen merged commit 74842b2 into phpbb:develop-ascraeus Jun 9, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@Pico Pico deleted the Pico:ticket/12606 branch Oct 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.