-
Notifications
You must be signed in to change notification settings - Fork 118
Update Supplier.php #242
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
Update Supplier.php #242
Conversation
add supplier.addressformat as configuration
@@ -179,7 +191,7 @@ public function setConfigFE( \Aimeos\MShop\Order\Item\Base\Service\Iface $orderS | |||
if( ( $code = $attributes['supplier.code'] ) != '' ) | |||
{ | |||
// add short address as attribute for summary page / customer email | |||
$attributes['supplier.address'] = $this->feConfig['supplier.code']['short'][$code]; | |||
$attributes['supplier.address'] = $this->feConfig['supplier.code'][$this->getConfigValue('supplier.addressformat')][$code]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use a default value if it's not set like:
$format = $this->getConfigValue( 'supplier.addressformat', 'short' );
$attributes['supplier.address'] = $this->feConfig['supplier.code'][$format][$code];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can i do it in oneline?
$attributes['supplier.address'] = $this->feConfig['supplier.code'][$this->getConfigValue('supplier.addressformat','short')][$code];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but the line will be very long and we try to keep the line length below 120 chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, okay, understand, i'll change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
We've discussed this a bit internally and came to the conclusion that adding just another format may not fit the requirements of many users as they may have different wishes for the address format depending on the country and/or intended use. Our recommendation would be to add the format string directly to the configuration instead of a fixed list of format options. |
make sense. |
Yes, your help would be valuable :-) |
create new PR for different approach. |
add supplier.addressformat as configuration
refer to #239
Sorry, i've screwed up the commits in previous PR. re-create this PR for clean code.