Hi , is there any reasons why "UtilTextService" instantiates directly in "PropelSchemaMerg

UKBF1R1S5

Hi <!channel>,
is there any reasons why "UtilTextService" instantiates directly in "PropelSchemaMerger"? I think a bridge that will be injected over constructor is better solution.

UK5DS29L2

Because it works , I guess

UK5DS29L2

IMO - If it works, it's OK. You won't blame people for taking another road to the store, just because you like another road more. Let programmers do it the way it works for them the best. 😉

UKBF1R1S5

I don't want to blame someone else.

Unknown

hi

Unknown

can you point me to some code?

Unknown

and i absolutely disagree with @UK5DS29L2 🙂

Unknown

of course there can always be a shortcut, but it should be a reflected decision. These things can make the life of people further down the road harder than necessary

UKN2CGHSB

https://github.com/spryker/propel/blob/master/src/Spryker/Zed/Propel/Business/Model/PropelSchemaMerger.php#L249

UKN2CGHSB

    private function getElementName(SimpleXMLElement $fromXmlChildElement, $tagName)
    {
        $elementName = (array)$fromXmlChildElement->attributes();
        $elementName = current($elementName);
        if (is_array($elementName) && isset($elementName['name'])) {
            $elementName = $tagName . '|' . $elementName['name'];
        }
        if (empty($elementName) || is_array($elementName)) {
            $utilTextService = new UtilTextService();
            $elementName = 'anonymous_' . $utilTextService->generateRandomString(32);
        }
        return $elementName;
    }

UKN2CGHSB

change was introduced in https://github.com/spryker/propel/commit/11b7a10f26896b1dd8430c09f9a0842c4387a6ec

stereomon committed on Dec 5, 2016

Andriy Netseplyayev

looks like that particular commit was about replacing the library with a service, and not “injecting it properly”. I think question from @UKBF1R1S5 is not to finger-point, but rather to understand something that doesn’t fit to the general picture. And I don’t see a good reason why not injecting it properly..

Unknown

i agree, this was probably a semi-automatic refactoring

Unknown

it is still unclean from Spryker side, should not look like that

UKN2CGHSB

sure, the line was touched (for service replacement) ~3½ years ago and nobody was taking any further care to check for new FooService() usages in business models..

As an answer to original question: "legacy code" could be the only reasonable explanation why proper injection was not used

Unknown

i agree, unfortunately

UKBF1R1S5

FYI. I created a pull request by spryker.