Skip to content
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

magento/magento2: Fixes for the schema cache.xsd #27307

Merged

Conversation

andrewbess
Copy link

Description (*)

This PR fixes the schema "cache.xsd" from Magento\Framework\Cache.

  1. The attribute "instance" has changed on required because if a developer won't be to use it new cache type will not be created;
  2. The parameters "label" and "description" have changed on required because of the class "Magento\Framework\App\Cache\TypeList" gets theirs without checking for existing. Please take a look at this code;
  3. Checking on unique of the type name has added;
  4. The configuration reader class has fixed for more informative during to run CLI commands;
  5. The class SchemaLocator has been fixed to check the cache configuration during configuration merging;
  6. The unnecessary checking has removed from the class TypeList;
  7. Unit tests have been covered.

Fixed Issues (if relevant)

Fixes: #26224

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@ghost
Copy link

ghost commented Mar 17, 2020

@andrewbess unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@magento-engcom-team magento-engcom-team added Component: App Component: Cache Release Line: 2.4 Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Mar 17, 2020
@lbajsarowicz
Copy link
Contributor

Just add WIP: prefix to your PR name.

@swnsma swnsma self-assigned this Mar 17, 2020
Copy link
Contributor

@swnsma swnsma left a comment

Choose a reason for hiding this comment

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

Failed function tests do not related to the improved functionality.

@magento-engcom-team
Copy link
Contributor

Hi @swnsma, thank you for the review.
ENGCOM-7114 has been created to process this Pull Request
✳️ @swnsma, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@lbajsarowicz
Copy link
Contributor

@swnsma It's not finished yet? :-)

@swnsma
Copy link
Contributor

swnsma commented Mar 17, 2020

@lbajsarowicz endless story :)

@andrewbess andrewbess changed the title magento/magento2: Fixes for the schema cache.xsd [DONT MERGE YET] magento/magento2: Fixes for the schema cache.xsd Mar 17, 2020
@andrewbess
Copy link
Author

Hello @lbajsarowicz
It has already finished.

/**
* Unit test of the cache configuration
*/
class ConfigTest extends \PHPUnit\Framework\TestCase
Copy link
Contributor

Choose a reason for hiding this comment

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

You can definately import the classes :-)

Copy link
Author

Choose a reason for hiding this comment

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

Hello @lbajsarowicz
Thank you for your remark.
I have fixed it. Please check.
Thank you in advance.

@ghost ghost assigned lbajsarowicz Mar 17, 2020
*/
public function schemaCorrectlyIdentifiesInvalidXmlDataProvider(): array
{
return include __DIR__ . '/_files/invalidCacheConfigXmlArray.php';
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting approach to provide code samples.

Copy link
Author

Choose a reason for hiding this comment

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

Hello @lbajsarowicz
Thank you for your remark.
Magento already has the same data providers. Please take a look at the screenshot
interesting-data-providers
Thank you in advance.

Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

Requesting changes due to use of deprecated MockObject. Please replace it with proper one :-)

*/
protected $_typeList;

/**
* @var \Magento\Framework\App\CacheInterface|\PHPUnit_Framework_MockObject_MockObject
* @var CacheInterface|PHPUnit_Framework_MockObject_MockObject
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with \PHPUnit\Framework\MockObject\MockObject

Copy link
Author

Choose a reason for hiding this comment

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

Hello @lbajsarowicz
Thank you for your remark.
I have fixed it. Please check.
Thank you in advance.

Comment on lines 31 to 33
* @param string $defaultScope
*/
public function __construct(
public function __construct( //phpcs:ignore Generic.CodeAnalysis.UselessOverridingMethod
Copy link
Contributor

Choose a reason for hiding this comment

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

Move that to DocBlock above.

Copy link
Author

Choose a reason for hiding this comment

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

Hello @lbajsarowicz
Thank you for your remark.
I have fixed it. Please check.
Thank you in advance.

Comment on lines 34 to 36
\Magento\Framework\Config\FileResolverInterface $fileResolver,
\Magento\Framework\Cache\Config\Converter $converter,
\Magento\Framework\Cache\Config\SchemaLocator $schemaLocator,
Copy link
Contributor

Choose a reason for hiding this comment

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

Import classes

Copy link
Author

Choose a reason for hiding this comment

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

Hello @lbajsarowicz
Thank you for your remark.
I have fixed it. Please check.
Thank you in advance.

Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

✔️ Thank you for your contribution

@magento-engcom-team
Copy link
Contributor

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7114 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@lbajsarowicz lbajsarowicz added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Mar 19, 2020
@andrewbess
Copy link
Author

Hello @lbajsarowicz
Perhaps, current PR is covered tests.
I have made the new test and fixed the existing test.

@swnsma swnsma added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests labels Mar 20, 2020
@swnsma
Copy link
Contributor

swnsma commented Mar 20, 2020

Integrity tests were introduced in this PR to check updated XSD schema.

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Before:
before

After:
after

@engcom-Echo
Copy link
Contributor

Failed static tests not related to the changes in this PR

@m2-assistant
Copy link

m2-assistant bot commented Mar 25, 2020

Hi @andrewbess, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants