Skip to content

#7241: Always add empty option for prefix and/or suffix if optional #11462

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 22, 2017
Merged

#7241: Always add empty option for prefix and/or suffix if optional #11462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
11e38f6
7241 Always add empty option for prefix and/or suffix if optional
avstudnitz Oct 14, 2017
7800e5d
7241 Re-add removed protected method and mark it as deprecated
avstudnitz Nov 15, 2017
252529f
Merge branch '2.2-develop' into 7241
avstudnitz Nov 15, 2017
519b805
7241 Change syntax of @see annotation for depracated method
avstudnitz Dec 1, 2017
eb79b38
7241 Undo creation of constants due to @api reasons
avstudnitz Dec 1, 2017
fce3afe
7241 Revert commit eb79b38 and re-add constants
avstudnitz Dec 1, 2017
3e38118
magento/magento2#7241: No option to start with blank option for prefi…
dmanners Dec 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions app/code/Magento/Config/Model/Config/Source/Nooptreq.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,19 @@
*/
class Nooptreq implements \Magento\Framework\Option\ArrayInterface
{
const VALUE_NO = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly while putting this to the 2.2-develop branch and with this class marked as @api we cannot add any new constants. For a PR to 2.3-develop we could add these new constants. For this PR we will have to change this back.

Copy link
Contributor Author

@avstudnitz avstudnitz Dec 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain why adding new constants is an issue? It doesn't affect backwards compatibility and only improves things, I don't see any disadavantage of that... sounds like a quite arbitrary rule to me, but perhaps it gets clearer if you explain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @dmanners in general, you are right. But in this particular, we can accept new constants. There are some cases when new constants can break client code, but the chances a very low.
@dmanners @avstudnitz thank you for collaboration

const VALUE_OPTIONAL = 'opt';
const VALUE_REQUIRED = 'req';

/**
* @return array
*/
public function toOptionArray()
{
return [
['value' => '', 'label' => __('No')],
['value' => 'opt', 'label' => __('Optional')],
['value' => 'req', 'label' => __('Required')]
['value' => self::VALUE_NO, 'label' => __('No')],
['value' => self::VALUE_OPTIONAL, 'label' => __('Optional')],
['value' => self::VALUE_REQUIRED, 'label' => __('Required')]
];
}
}
14 changes: 10 additions & 4 deletions app/code/Magento/Customer/Block/Widget/Name.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,11 @@ public function getPrefixOptions()
$prefixOptions = $this->options->getNamePrefixOptions();

if ($this->getObject() && !empty($prefixOptions)) {
$oldPrefix = $this->escapeHtml(trim($this->getObject()->getPrefix()));
$prefixOptions[$oldPrefix] = $oldPrefix;
$prefixOption = $this->getObject()->getPrefix();
$oldPrefix = $this->escapeHtml(trim($prefixOption));
if ($prefixOption !== null && !isset($prefixOptions[$oldPrefix]) && !isset($prefixOptions[$prefixOption])) {
$prefixOptions[$oldPrefix] = $oldPrefix;
}
}
return $prefixOptions;
}
Expand Down Expand Up @@ -161,8 +164,11 @@ public function getSuffixOptions()
{
$suffixOptions = $this->options->getNameSuffixOptions();
if ($this->getObject() && !empty($suffixOptions)) {
$oldSuffix = $this->escapeHtml(trim($this->getObject()->getSuffix()));
$suffixOptions[$oldSuffix] = $oldSuffix;
$suffixOption = $this->getObject()->getSuffix();
$oldSuffix = $this->escapeHtml(trim($suffixOption));
if ($suffixOption !== null && !isset($suffixOptions[$oldSuffix]) && !isset($suffixOptions[$suffixOption])) {
$suffixOptions[$oldSuffix] = $oldSuffix;
}
}
return $suffixOptions;
}
Expand Down
32 changes: 29 additions & 3 deletions app/code/Magento/Customer/Model/Options.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
namespace Magento\Customer\Model;

use Magento\Config\Model\Config\Source\Nooptreq as NooptreqSource;
use Magento\Customer\Helper\Address as AddressHelper;
use Magento\Framework\Escaper;

Expand Down Expand Up @@ -42,7 +43,10 @@ public function __construct(
*/
public function getNamePrefixOptions($store = null)
{
return $this->_prepareNamePrefixSuffixOptions($this->addressHelper->getConfig('prefix_options', $store));
return $this->prepareNamePrefixSuffixOptions(
$this->addressHelper->getConfig('prefix_options', $store),
$this->addressHelper->getConfig('prefix_show', $store) == NooptreqSource::VALUE_OPTIONAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we will need to change this since we cannot add the new constants.

);
}

/**
Expand All @@ -53,16 +57,34 @@ public function getNamePrefixOptions($store = null)
*/
public function getNameSuffixOptions($store = null)
{
return $this->_prepareNamePrefixSuffixOptions($this->addressHelper->getConfig('suffix_options', $store));
return $this->prepareNamePrefixSuffixOptions(
$this->addressHelper->getConfig('suffix_options', $store),
$this->addressHelper->getConfig('suffix_show', $store) == NooptreqSource::VALUE_OPTIONAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we will need to change this since we cannot add the new constants.

);
}

/**
* @param $options
* @param bool $isOptional
* @return array|bool
*
* @deprecated
* @see prepareNamePrefixSuffixOptions()
*/
protected function _prepareNamePrefixSuffixOptions($options, $isOptional = false)
{
return $this->prepareNamePrefixSuffixOptions($options, $isOptional);
}

/**
* Unserialize and clear name prefix or suffix options
* If field is optional, add an empty first option.
*
* @param string $options
* @param bool $isOptional
* @return array|bool
*/
protected function _prepareNamePrefixSuffixOptions($options)
private function prepareNamePrefixSuffixOptions($options, $isOptional = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

As part of the BC guidelines you cannot remove a protected method http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/#public-and-protected-method-removal I would suggest that you keep the private method and the internal calls to this private method and then simply add back the protected method that simply calls the private method and mark it as deprecated.

{
$options = trim($options);
if (empty($options)) {
Expand All @@ -74,6 +96,10 @@ protected function _prepareNamePrefixSuffixOptions($options)
$value = $this->escaper->escapeHtml(trim($value));
$result[$value] = $value;
}
if ($isOptional && trim(current($options))) {
$result = array_merge([' ' => ' '], $result);
}

return $result;
}
}
4 changes: 2 additions & 2 deletions app/code/Magento/Customer/etc/adminhtml/system.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@
<field id="prefix_options" translate="label comment" sortOrder="30" showInDefault="1" showInWebsite="1" showInStore="0">
<label>Prefix Dropdown Options</label>
<comment>
<![CDATA[Semicolon (;) separated values.<br/>Put semicolon in the beginning for empty first option.<br/>Leave empty for open text field.]]>
<![CDATA[Semicolon (;) separated values.<br/>Leave empty for open text field.]]>
</comment>
</field>
<field id="middlename_show" translate="label comment" type="select" sortOrder="40" showInDefault="1" showInWebsite="1" showInStore="0">
Expand All @@ -228,7 +228,7 @@
<field id="suffix_options" translate="label comment" sortOrder="60" showInDefault="1" showInWebsite="1" showInStore="0">
<label>Suffix Dropdown Options</label>
<comment>
<![CDATA[Semicolon (;) separated values.<br/>Put semicolon in the beginning for empty first option.<br/>Leave empty for open text field.]]>
<![CDATA[Semicolon (;) separated values.<br/>Leave empty for open text field.]]>
</comment>
</field>
<field id="dob_show" translate="label" type="select" sortOrder="70" showInDefault="1" showInWebsite="1" showInStore="0">
Expand Down
4 changes: 2 additions & 2 deletions app/code/Magento/Customer/i18n/en_US.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,9 +479,9 @@ Strong,Strong
"The title that goes before name (Mr., Mrs., etc.)","The title that goes before name (Mr., Mrs., etc.)"
"Prefix Dropdown Options","Prefix Dropdown Options"
"
Semicolon (;) separated values.<br/>Put semicolon in the beginning for empty first option.<br/>Leave empty for open text field.
Semicolon (;) separated values.<br/>Leave empty for open text field.
","
Semicolon (;) separated values.<br/>Put semicolon in the beginning for empty first option.<br/>Leave empty for open text field.
Semicolon (;) separated values.<br/>Leave empty for open text field.
"
"Show Middle Name (initial)","Show Middle Name (initial)"
"Always optional.","Always optional."
Expand Down