.

CommentFileSizeAuthor
#13 views-1763678-13.patch263.11 KBtim.plunkett
#9 views-1763678-9.patch254.54 KBdawehner
#5 views-1763678-5.patch253.67 KBdawehner
#3 views-1763678-3.patch251.87 KBdawehner
#1 views-1763678-1.patch251.49 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
views-1763678-1.patch251.49 KB

TODO:

* Review the patch for myself
* Check which methods should be protected (for example optionsDefinition might be a good candidate for that)

Needs review for the testbot, some of the tests runned fine on my local system.

Status: Needs review » Needs work

The last submitted patch, views-1763678-1.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
views-1763678-3.patch251.87 KB

Changed public to protected for:

  • optionDefinition
  • unpackTranslatable (singular)
  • exportOption (singular)

Fixed the test failure

Status: Needs review » Needs work

The last submitted patch, views-1763678-3.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
views-1763678-5.patch253.67 KB

Fixed some of the bugs

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

Looks good to me I did a "quick" review and it looks ok. I didn't read every single change very carefully as it is to huge to read through slowly.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Reviewed & tested by the community » Needs work 
+++ b/lib/Drupal/views/Plugin/views/PluginBase.phpundefined
@@ -120,13 +120,13 @@ abstract class PluginBase extends ComponentPluginBase {
-  function unpack_options(&$storage, $options, $definition = NULL, $all = TRUE, $check = TRUE, $localization_keys = array()) {
+  public function unpackOptions(&$storage, $options, $definition = NULL, $all = TRUE, $check = TRUE, $localization_keys = array()) {

@@ -389,29 +389,29 @@ abstract class PluginBase extends ComponentPluginBase {
-  function options_submit(&$form, &$form_state) { }
+  public function optionsSubmit(&$form, &$form_state) { }

I think fooOptions makes more sense than optionsFoo

It'd be nice to standardize since we're renaming anyway.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

I think these should be:

exportOptions
buildOptionsForm
buildOptionsSummaryForm
submitOptions
validateOptions
unpackOptions
defineOptions

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
views-1763678-9.patch254.54 KB

Suggestion of merlinofchaos:

How about handleOptionsForm, handleOptionsFormValidate and handleOptionsFormSubmit?

Another possibility

buildOptionsForm
submitOptionsForm
validateOptionsForm
defineOptions

The attached patch contains the last suggestion.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, views-1763678-9.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
Issue tags: +VDC

#9: views-1763678-9.patch queued for re-testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

For me the namind is ok

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1763678-13.patch263.11 KB

Changed in this patch:

validate_arg        -> validateArgument
validate_fail       -> validateFail
validate_valid_time -> validateValidTime
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Fixed

http://drupalcode.org/project/views.git/commit/1f871af
Awesome!

Automatically closed -- issue fixed for 2 weeks with no activity.