Make custom store address formatting possible

[jokeputs]

Description

It's not possible to change the format of the store address used in for example the footer of an e-mail template. At least not in a clean way. If the address renderer would be an interface it would make it possible to create a custom renderer and inject it using a preference.

This is not a bug in Magento but it would make customisation easier.

[orlangur]

Such interface should have been named something like AddressRendererInterface or AddressFormatterInterface.

[jokeputs]

I did considered this but because the interface is in the namespace Magento\Store\Model\Address it seemed unnecessary to add Address to the name. What do you think?

[orlangur]

The rule of thumb is (imho) - name should be verbose enough for understanding without aliasing. We already have generic RendererInterface which is fine as it just renders element. But here renderer is more specific.

Another example - Collection lying inside Product folder is not a good name, ProductCollection is much better. But there is no need to duplicate context in method names, $address->getId() is just fine, $address->getAddressId() looks inelegantly.

Anyway, this is just a side note, I believe we don't need a new interface introduced here.

[orlangur]

I don't see any difference between existing and proposed implementation from customization perspective. One can easily swap Renderer implementation injected into \Magento\Store\Model\Information::__construct in both of them.

It's not possible to change the format of the store address used in for example the footer of an e-mail template. At least not in a clean way.

In fact it's easy in more than one way, using custom Renderer implementation or creating a plugin.

However, what I really don't like in current implementation is template hardcoded in \Magento\Store\Model\Address\Renderer::format. I would make it a constructor argument with current value as default so that it can be easily changed via di.xml. What do you think?

[jokeputs]

@orlangur, I tried a plugin to add my own template but I don't like that solution. I could use an around or after plugin but in both cases it's unnecessary to run the original code because I'll change the output anyway.
If I create a custom implementation for Renderer that is injected into \Magento\Store\Model\Information::__construct my implementation still need to inherit from Renderer because this is what the constructor expects. It seems weird to inherit from a class that only has one public method when that method needs to be overridden. An interface seems like a better solution.

I agree, the template should not be hard coded in the first place. It would be nice if you could define the template in configuration. This is possible for the billing and shipping address.
An interface just seemed like a solution that didn't involve changing to much code and possibly breaking things.

[orlangur]

It seems weird to inherit from a class that only has one public method when that method needs to be overridden. An interface seems like a better solution.

To me it seems weird to introduce interfaces for such cases, as it means that we will introduce an interface for every single class injected.

An interface just seemed like a solution that didn't involve changing to much code and possibly breaking things.

It is not BC policy compliant in fact as it could break existing customizations, please check http://devdocs.magento.com/guides/v2.1/contributor-guide/backward-compatible-development/index.html#modifying-the-method-argument-type.

solution that didn't involve changing to much code and possibly breaking things.

My suggestion is as simple as

public function __construct(
        EventManager $eventManager,
        FilterManager $filterManager,
        $template = "{{var name}}\n{{var street_line1}}\n{{depend street_line2}}{{var street_line2}}\n{{/depend}}" .
            "{{depend city}}{{var city}},{{/depend}} {{var region}} {{depend postcode}}{{var postcode}},{{/depend}}\n" .
            "{{var country}}"
    ) {
        ...
    }

It would be nice if you could define the template in configuration. This is possible for the billing and shipping address

I didn't have a chance to check billing/shipping implementation, please make address renderer consistent with them.

[jokeputs]

@orlangur , I'll have a look or I can make a different PR with the configurable template. Thanks for all the feedback.

[orlangur]

You welcome 👍 For the same issue it's better to continue in this PR, just force push the necessary changes.

[dmanners]

Hi @jokeputs I had the same issue the other day when trying to replace another part of Magento that did not use an interface. I would love to see this updated like you have done to include an interface and thus make replacement easier.

@orlangur maybe I am missing something but I cannot see how this would actually break BC as long as the interface defines all the public methods that are in the implementation it should be ok. If someone has added a preference to the implementation they would have to extend it anyway and so would automatically also implement the interface. I could easily be missing something here though.

[dmanners]

Ah now I see it @orlangur if someone has extended app/code/Magento/Store/Model/Information.php and has a new __construct you would end up with warnings on Declaration of X should be compatible with Y. For example:

<?php

class BaseClass implements BaseInterface
{
    public function saySomething(TotalInterface $collector)
    {
        echo 'say my name out loud punk';
    }
}

class ChildClass extends BaseClass
{
    public function saySomething(TotalCollector $collector)
    {
        echo 'say my name punk';
    }
}

class TotalCollector implements TotalInterface
{
}

interface BaseInterface
{
}

interface TotalInterface
{
}

$collectorObj = new TotalCollector();

$childObj = new ChildClass();
$childObj->saySomething($collectorObj);

$baseObj = new BaseClass();
$baseObj->saySomething($collectorObj);

Gives you: Warning: Declaration of ChildClass::saySomething(TotalCollector $collector) should be compatible with BaseClass::saySomething(TotalInterface $collector) in /in/tbEtL on line 11

[orlangur]

trying to replace another part of Magento that did not use an interface
to include an interface and thus make replacement easier

You can replace the whole class (a.k.a. class rewrite), you can inject another implementation as an argument for this particular class - I don't see how interface makes the life easier.

[dmanners]

For me at least having the interface makes it "easier" (maybe easier is the wrong word here but I am not sure what the right word is) as you can stick with composition rather than inheritance. If you do not have the interface then you new version must extend to original even if you do not need anything from the original. Some cases this will be no issue and might even be the desired option but there are others that it is not needed.

The example I was working on the other day was to completely replace the totalscollector. Our system needed to work with totals in it's own way. But since the collector is called via the implementation rather than the interface my class needed to extend the original, even if the task meant that I needed 0% of the original class. Again not sure if this could be described as "easier" but for me it just sits better with the way I develop. Honestly both options work and are fine, I just prefer the interface approach.

[orlangur]

@dmanners thanks, I see your point. As to me super-abstract classes like AbstractAction or AbstractModel are a big problem and need to be replaced with inheritance-less implementation.

With current implementation to make it more clear that you only use class interface and not implementation one can replace ALL parent methods with empty stubs.

For the problem described approach I proposed looks much cleaner to me and does not require new interface introduction.

For the general case, where do you think should be the line when we stop to convert each class into interface in constructor? Or more interfaces = the better and at some point we will rely only on interfaces?

[orlangur]

@jokeputs are you going to perform suggested changes?

[jokeputs]

@orlangur, I will but I haven't had the time yet.

[orlangur]

@jokeputs sure thing, there is no hurry, sorry for bothering.

[jokeputs]

@orlangur, I've updated my pull request. The address template now gets injected.

[orlangur]

Can we have it as a DEFAULT_TEMPLATE constant in original class maybe? And then just specify it as a default value of $template argument.

[jokeputs]

I've updated the pull request to include the constant.

[orlangur]

Hello @jokeputs, internal builds passed successfully, please remove the app/code/Magento/Store/etc/di.xml changes as they are not necessary now and force push squashing all changes into single commit (please let me know if you need any assistance with that).

[orlangur]

Thanks! Could you make branch history clear now? Something similar to #11117 (comment) should do the trick.

[jokeputs]

@orlangur, I'm trying to follow your instruction but I'm not sure how I should get the squashed commit on my own branch. In #11117 the change is committed in the develop branch but this pull request is from my own branch store-address-renderer-interface.
This is the first time I'm doing this. I'm probably missing something obvious.

[orlangur]

@jokeputs just git checkout store-address-renderer-interface and create a backup branch with arbitrary name instead of develop-old. If you just replace "develop" to "store-address-renderer-interface" in all steps it should work according to my understanding.

[jokeputs]

@orlangur,
I get stuck at step 4.

1. git checkout store-address-renderer-interfac
2. git checkout -b store-address-renderer-interface-dummy
3. git branch -D store-address-renderer-interface
4. git checkout upstream/store-address-renderer-interface

https://github.com/magento/magento2/ is called upstream with me. When I run the command I get the error error: pathspec 'upstream/store-address-renderer-interface' did not match any file(s) known to git. which makes sense because the branch doesn't exist in the original repository.
If I replace upstream/store-address-renderer-interface with upstream/develop I can create the squashed commit but on the developbranch.

Any ideas?

[orlangur]

That's correct as jokeputs:store-address-renderer-interface is only present in your form, not in upstream.

If I replace upstream/store-address-renderer-interface with upstream/develop I can create the squashed commit but on the developbranch.

Yeah, that was what I was asking for. Not on develop branch but on the new branch created from develop.

Ideally it would be nice to change target of this PR to 2.2-develop also. Having a branch with these changes as a squashed commit, you can just cherry-pick it to the new branch created from 2.2-develop.

[orlangur]

There is no need to close PR to change target branch.

You can leave the one commit branch targeting develop if you wish, it's not a problem for processing.

[jokeputs]

My source branch jokeputs:store-address-renderer-interface was created from develop and also had the non-squashed commits. I deleted that one to create a new branch from 2.2-develop with the cherry-picked commit but I didn't realise that would close the pull request.

Anyway, I have a clean branch now with the one commit. Should I just create a new pull request for that one? Or can I somehow change this pull request?

[orlangur]

Just push clean branch as store-address-renderer-interface back to your fork. Hope it will allow to reopen.

I deleted that one to create a new branch from 2.2-develop with the cherry-picked commit but I didn't realise that would close the pull request.

Hm, didn't know that as well, I usually use force push instead of branch deletion.

[jokeputs]

Just push clean branch as store-address-renderer-interface back to your fork. Hope it will allow to reopen.

Hm, that doesn't seem to work. I've got a "new" store-address-renderer-interface branch but this pull request doesn't seem to pick it up.

[orlangur]

True, https://github.com/jokeputs/magento2/tree/store-address-renderer-interface is there but it does not satisfy GitHub.

Please post a new PR targeting 2.2-develop and I'll pickup it up immediately.

[jokeputs]

Ok, will do. Thanks for all the help!