Joeytje50 has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/361635 )
Change subject: Implement suggested changes code-review
......................................................................
Implement suggested changes code-review
1) changed some syntax that only works in PHP5.4+ to the more supported
older syntax
2) Split up some comments to ensure they cut off at around 80 characters
width, assuming tab width of 4 spaces.
3) Elaborated on a comment block
4) Fixed an issue with a non-class variable being used in two different
functions (fixing a bug)
Change-Id: Iabbcf9d3c56dc711facb4d1fbab5baccb660fb2b
---
M includes/PF_FormPrinter.php
M includes/PF_TemplateInForm.php
2 files changed, 67 insertions(+), 50 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/PageForms
refs/changes/35/361635/1
diff --git a/includes/PF_FormPrinter.php b/includes/PF_FormPrinter.php
index 6baa6e8..3767736 100644
--- a/includes/PF_FormPrinter.php
+++ b/includes/PF_FormPrinter.php
@@ -50,6 +50,7 @@
// keep track of the position of brackets within tag processing
private $brackets_loc;
private $brackets_end_loc;
+ private $generated_page_name;
public function __construct() {
global $wgPageFormsDisableOutsideServices;
@@ -709,15 +710,16 @@
} else {
$this->existing_page_content =
$this->strReplaceFirst( $existing_template_text, '',
$this->existing_page_content );
}
- // If we've found a match in the source page,
there's a good chance that this
- // page was created with this form - note that,
so we don't send the user a warning.
+ // If we've found a match in the source page,
there's a good
+ // chance that this page was created with this
form - note
+ // that, so we don't send the user a warning.
$this->source_page_matches_this_form = true;
}
}
// We get values from the request, regardless of whether the
the source
- // is the page or a form submit, because even if the source is
a page, values
- // can still come from a query string.
+ // is the page or a form submit, because even if the source is
a page,
+ // values can still come from a query string.
$this->tif->setFieldValuesFromSubmit();
$this->tif->checkIfAllInstancesPrinted( $this->form_submitted,
$this->source_is_page );
@@ -743,10 +745,12 @@
// handles {{{field}}}
private function tagField() {
global $wgUser, $wgParser;
- global $wgPageFormsTabIndex; // used to represent the current
tab index in the form
+ global $wgPageFormsTabIndex; // used to represent the current
tab index
+ // in
the form
global $wgPageFormsFieldNum; // used for setting various HTML
IDs
- // If the template is null, that (hopefully) means we're
handling the free text field.
- // Make the template a dummy variable.
+
+ // If the template is null, that (hopefully) means we're
handling the
+ // free text field; make the template a dummy variable.
if ( $this->tif == null ) {
$this->template = new PFTemplate( null, array() );
$this->tif = new PFTemplateInForm();
@@ -767,36 +771,43 @@
$this->placeholderFields[] = self::placeholderFormat(
$this->tif->getTemplateName(), $field_name );
}
- if ($val_modifier !== null) {
- $page_value =
$this->tif->getValuesFromPage()[$field_name];
+ if ( $val_modifier !== null ) {
+ $page_value_temp = $this->tif->getValuesFromPage();
+ $page_value = $page_value_temp[$field_name];
}
- if ($val_modifier === '+') {
- if (preg_match("#(,|\^)\s*$cur_value\s*(,|\$)#",
$page_value) === 0) {
- if (trim($page_value) !== '') {
- // if page_value is empty, simply don't
do anything, because then cur_value
- // is already the value it has to be
(no commas needed).
+ if ( $val_modifier === '+' ) {
+ if ( preg_match( "#(,|\^)\s*$cur_value\s*(,|\$)#",
$page_value ) === 0 ) {
+ if ( trim( $page_value ) !== '' ) {
+ // if page_value is empty, simply don't
do anything, because
+ // then cur_value is already the value
it has to be
+ // (no commas needed).
$cur_value = "$page_value,$cur_value";
}
} else {
$cur_value = $page_value;
}
- } elseif ($val_modifier === '-') {
+ } elseif ( $val_modifier === '-' ) {
// get an array of elements to remove:
- $remove = array_map('trim', explode(",", $cur_value));
+ $remove = array_map( 'trim', explode( ",", $cur_value )
);
// process the current value:
- $val_array = array_map('trim', explode(",",
$page_value));
+ $val_array = array_map( 'trim', explode( ",",
$page_value ) );
// remove element(s) from list
- foreach ($remove as $rmv) {
+ foreach ( $remove as $rmv ) {
// go through each element and remove match(es)
- if (($key = array_search($rmv, $val_array)) !==
false) {
- unset($val_array[$key]);
+ if ( ( $key = array_search( $rmv, $val_array )
) !== false ) {
+ unset( $val_array[$key] );
}
}
- // Convert modified array back to a comma-separated
string value and modify
- $cur_value = implode(",", $val_array);
- if ($cur_value === '') {
- // HACK: setting an empty string prevents
anything from happening at all.
- // set a dummy string that evaluates to an
empty string
+ // Convert modified array back to a comma-separated
string value
+ // and modify
+ $cur_value = implode( ",", $val_array );
+ if ( $cur_value === '' ) {
+ // HACK: setting an empty string prevents
anything from
+ // happening at all (ie. the value doesn't
change on the
+ // target page). Set a dummy string that
evaluates to an
+ // empty string. This makes it possible to
remove the last
+ // value in a list by using the -= 'operator'
(leaving the
+ // parameter value completely empty).
$cur_value = '{{subst:lc: }}';
}
}
@@ -876,14 +887,15 @@
}
}
} else {
- // If it's not a list, it's probably
from a checkbox or date input -
- // convert the values into a string.
+ // If it's not a list, it's probably
from a checkbox or
+ // date input - convert the values into
a string.
$cur_value_in_template =
self::getStringFromPassedInArray( $cur_value, $delimiter );
}
} elseif ( $form_field->holdsTemplate() ) {
- // If this field holds an embedded template,
and the value is not an array, it means
- // there are no instances of the template - set
the value to null to avoid getting
- // whatever is currently on the page.
+ // If this field holds an embedded template,
and the value is
+ // not an array, it means there are no
instances of the template;
+ // set the value to null to avoid getting
whatever is currently
+ // on the page.
$cur_value_in_template = null;
} else { // value is not an array
$cur_value_in_template = $cur_value;
@@ -892,15 +904,15 @@
// If we're creating the page name from a formula based
on
// form values, see if the current input is part of
that formula,
// and if so, substitute in the actual value.
- if ( $this->form_submitted && $generated_page_name !==
'' ) {
+ if ( $this->form_submitted &&
$this->generated_page_name !== '' ) {
// This line appears to be unnecessary.
- // $generated_page_name = str_replace('.', '_',
$generated_page_name);
- $generated_page_name = str_replace( ' ', '_',
$generated_page_name );
+ // $this->generated_page_name =
str_replace('.', '_', $this->generated_page_name);
+ $this->generated_page_name = str_replace( ' ',
'_', $this->generated_page_name );
$escaped_input_name = str_replace( ' ', '_',
$form_field->getInputName() );
- $generated_page_name = str_ireplace(
"<$escaped_input_name>", $cur_value_in_template, $generated_page_name );
+ $this->generated_page_name = str_ireplace(
"<$escaped_input_name>", $cur_value_in_template, $this->generated_page_name );
// Once the substitution is done, replace
underlines back
// with spaces.
- $generated_page_name = str_replace( '_', ' ',
$generated_page_name );
+ $this->generated_page_name = str_replace( '_',
' ', $this->generated_page_name );
}
// Call hooks - unfortunately this has to be split into
two separate
@@ -1077,8 +1089,8 @@
$section_start_loc = 0;
if ( $this->source_is_page && $this->existing_page_content !==
null ) {
// For the last section of the page, there is no
trailing newline in
- // $this->existing_page_content, but the code below
expects it. This code
- // ensures that there is always trailing newline. T72202
+ // $this->existing_page_content, but the code below
expects it.
+ // This code ensures that there is always trailing
newline. T72202
if ( substr( $this->existing_page_content, -1 ) !==
"\n" ) {
$this->existing_page_content .= "\n";
}
@@ -1201,7 +1213,7 @@
// existing article as well, finding template and field
// declarations and replacing them with form elements, either
// blank or pre-populated, as appropriate.
- private function processDefinitions($form_def_sections) {
+ private function processDefinitions( $form_def_sections ) {
$this->template_name = null;
$this->template = null;
$this->tif = null;
@@ -1298,14 +1310,17 @@
// The normal process.
$this->form_text .=
$multipleTemplateHTML;
} else {
- // The template text won't be appended
at the end of the template like for
- // usual multiple template forms. The
HTML text will instead be stored in
- // the $multipleTemplateHTML variable,
and then added in the right
- // @insertHTML_".$placeHolderField."@";
position Optimization: actually, instead of
- // separating the processes, the usual
multiple template forms could also be
- // handled this way if a fitting
placeholder tag was added.
- // We replace the HTML into the current
placeholder tag, but also add another
- // placeholder tag, to keep track of it.
+ // The template text won't be appended
at the end of the
+ // template like for usual multiple
template forms. The HTML
+ // text will instead be stored in the
$multipleTemplateHTML
+ // variable, and then added in the right
+ // @insertHTML_".$placeHolderField."@";
position
+ // Optimization: actually, instead of
separating the
+ // processes, the usual multiple
template forms could also
+ // be handled this way if a fitting
placeholder tag was
+ // added. We replace the HTML into the
current placeholder
+ // tag, but also add another
placeholder tag, to keep track
+ // of it.
$multipleTemplateHTML .=
self::makePlaceholderInFormHTML( $placeholder );
$this->form_text = str_replace(
self::makePlaceholderInFormHTML( $placeholder ), $multipleTemplateHTML,
$this->form_text );
}
@@ -1361,7 +1376,7 @@
$wgPageFormsFieldNum = 0;
$this->source_page_matches_this_form = false;
$form_page_title = null;
- $generated_page_name = $page_name_formula;
+ $this->generated_page_name = $page_name_formula;
// $this->form_is_partial is true if:
// (a) 'partial' == 1 in the arguments
// (b) 'partial form' is found in the form definition
@@ -1497,7 +1512,7 @@
} // end while
$form_def_sections[] = trim( substr( $form_def, $section_start
) );
- $this->processDefinitions($form_def_sections);
+ $this->processDefinitions( $form_def_sections );
// Cleanup - everything has been browsed.
// Remove all the remaining placeholder
@@ -1615,7 +1630,7 @@
// $wgParser = $oldParser;
- return array( $this->form_text, $page_text, $form_page_title,
$generated_page_name );
+ return array( $this->form_text, $page_text, $form_page_title,
$this->generated_page_name );
}
/**
diff --git a/includes/PF_TemplateInForm.php b/includes/PF_TemplateInForm.php
index 263256e..f6f16d2 100644
--- a/includes/PF_TemplateInForm.php
+++ b/includes/PF_TemplateInForm.php
@@ -244,7 +244,9 @@
// Also replace periods with underlines, since that's what
// POST does to strings anyway.
$query_template_name = str_replace( '.', '_',
$query_template_name );
- // ...and escape apostrophes. (Or don't.)
+ // ...and escape apostrophes.
+
+ // (Or don't.)
//$query_template_name = str_replace( "'", "\'",
$query_template_name );
$allValuesFromSubmit = $wgRequest->getArray(
$query_template_name );
--
To view, visit https://gerrit.wikimedia.org/r/361635
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iabbcf9d3c56dc711facb4d1fbab5baccb660fb2b
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/PageForms
Gerrit-Branch: master
Gerrit-Owner: Joeytje50 <joeytj...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
