Skip to content

Conversation

phoenix128
Copy link
Collaborator

@phoenix128 phoenix128 commented Jul 31, 2019

MageSpecialist Notifier modules for Magento 2

@hostep
Copy link

hostep commented Aug 1, 2019

@phoenix128, just FYI, the composer version constraint ^7.2|^7.3 for php is not really correct here I think, because it boils down to: 7.2.0 ... 7.9999.9999.
And it would be the exact same if you would have used ^7.2 or ^7.2.0

What you probably want is: ~7.2.0 || ~7.3.0 which means: 7.2.0 ... 7.3.9999
(I'm also using double pipes as that's the more correct meaning of or in these constraints, the single pipe was recommended many years ago but is now no longer recommended, but it also won't disappear in the foreseeable future)

More info: https://getcomposer.org/doc/articles/versions.md

@phoenix128
Copy link
Collaborator Author

@hostep ,
ops, you are totally right! I am fixing it.

/**
* Adapter code for email adapter
*/
public const ADAPTER_CODE = 'email';
Copy link

Choose a reason for hiding this comment

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

Does it make sense to mark these constants as public?

Copy link
Collaborator Author

@phoenix128 phoenix128 Aug 26, 2019

Choose a reason for hiding this comment

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

It is referenced here for example: \MSP\NotifierEmailAdapterAdminUi\Ui\DataProvider\Form\Channel\Modifier\Email the idea is to create a hard dependency through constants when a specific adapter is required.

@magento-cicd2
Copy link

magento-cicd2 commented Aug 20, 2019

CLA assistant check
All committers have signed the CLA.

@sidolov sidolov merged commit 572553c into magento:2.3-develop Oct 31, 2019
mmansoor-magento pushed a commit that referenced this pull request Oct 22, 2020
[Arrows] MC-34292: AdminCreateUserRoleWithReportsActionGroup needs to be refactored to be based in CE
magento-devops-reposync-svc pushed a commit that referenced this pull request May 24, 2022
CABPI-397: Fix Failed Functional Tests CE Tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants