Replace into the layout adminhtml_order_shipment_new.xml block alias …

[sylvainraye]

…attribute with block name attribute to allow template overrides.

[vrann]

@diglin thanks for contribution.
Can you explain why this change is needed and describe how to reproduce result?

[sylvainraye]

@vrann it's needed otherwise you can't overwrite the renderer template properly. You can't get a block via its alias but only via its name. A block having an alias but no name, receive an anonymous block name like $parentNode->getElementName() . '_schedule_block' + VALUE as referenced at \Magento\Framework\View\Layout\ScheduledStructure\Helper::scheduleStructure
It's difficult to be sure that this name doesn't change as soon as other block are created into the parent block.

If a developer wants to customize or change the renderer block, he can't because <referenceBlock> works only with name not with alias.

Thanks to the commited changes, a developer can extends/customize the renderer like that

In File adminhtml_order_shipment_new.xml:

        <referenceBlock name="default"><!-- without the fix the block anonymous name  may be like order_items_schedule_block3 -->
            <action method="setTemplate">
                <argument name="template" xsi:type="string">Diglin_BasePriceUnit::shipping/create/items/renderer/default.phtml</argument>
            </action>
        </referenceBlock>

[vrann]

@diglin Thank you for the explanation; it makes sense.

[vrann]

@diglin can you please come up with unique names for the blocks? "bundle" and "default" are pretty generic and there are high chances that another block with the same name will call collision. Block names should be unique within context.

Something like "sales_order_bundle_renderrer" and "sales_order_bundle_default" will work

[sylvainraye]

@vrann I agree hw that's what comes from core code. I didn't changed anything, I just setup a replacement of as to name. Should I update my pull request?

[vrann]

@diglin yes, please, go ahead.
Those tokens makes sense as aliases, but for names they are too generic.

[sylvainraye]

@vrann done :-)

[vrann]

@diglin change looks good, accepting

[okorshenko]

@diglin Thank you for your contribution to Magento 2 project! Your pull request has been successfully merged!

[YevSent]

@okorshenko, @vrann, @diglin this fix is incorrect and causes a fatal error. Try to create shipment for an order placed with a simple or bundle product. If specified rendered doesn't exist, the default must be used, but you changed the default name to shipment specific and now it doesn't work.

[orlangur]

@joni-jones integration test demonstrating problem would be really nice

[YevSent]

@orlangur, follow functional tests: Magento\Shipping\Test\TestCase\CreateShipmentEntityTest, Magento\Shipping\Test\TestCase\SalesShippingReportEntityTest, Magento\Sales\Test\TestCase\MassOrdersUpdateTest can be used.

[orlangur]

No-no, I'm not saying the issue is hard to reproduce. Just would be nice to enforce correct behavior with mandatory test.

Are these functional tests failing now after merge?

[YevSent]

Yes, these tests are failed now.

[orlangur]

Dunno why it was merged then :-| Could you report new issue referring to this PR? Especially mentioning functional tests you found broken.

Don't want to arrogate your credits ;)

[YevSent]

Mentioned tests don't present in required test plans. We have created internal ticket (MAGETWO-64909) and fix will be merged ASAP.

[orlangur]

Ah, ok, didn't notice you are a member of Magento crew ("Member" label indicated this some time ago, now I see only "Contributor"s).

Too bad full functional tests suite is still not mandatory or at least such an important cases not covered with integration tests. Thanks for quick reaction 👍