Skip to content

feat: add functionality for plugin checksums #5362

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

Closed
wants to merge 1 commit into from

Conversation

M-arcus
Copy link
Contributor

@M-arcus M-arcus commented Oct 31, 2024

1. Why is this change necessary?

Plugin developers have no way to see if their plugin code has been modified other than checking each file. This functionality was included in Shopware 5.

2. What does this change do, exactly?

It adds 2 commands and a modal in the administration that allows the creation and checking of a checksum.

3. Describe each step to reproduce the issue or behavior.

4. Please link to the relevant issues (if any).

5. Checklist

  • I have rebased my changes to remove merge conflicts
  • I have written tests and verified that they fail without my change
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfill them.

Copy link

Warnings
⚠️ The Pull Request doesn't contain any changelog file
⚠️ Please be kind and add unit tests for your new code in these files:
src/Core/Framework/Plugin/Command/Lifecycle/PluginChecksumCheckCommand.php
src/Core/Framework/Plugin/Command/Lifecycle/PluginChecksumCheckResult.php
src/Core/Framework/Plugin/Command/Lifecycle/PluginChecksumCreateCommand.php
src/Core/Framework/Plugin/Command/Lifecycle/PluginFileHashService.php
src/Core/Framework/Store/Services/ExtensionChecksumLoader.php
If you are sure everything is fine with your changes, you can resolve this warning.
You can run `composer make:coverage` to generate dummy unit tests for files that are not covered

@M-arcus
Copy link
Contributor Author

M-arcus commented Oct 31, 2024

I will add the administration files, the changelog file and tests within the next few days.

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 0% with 140 lines in your changes missing coverage. Please review.

Project coverage is 43.83%. Comparing base (21cee05) to head (6904b1d).
Report is 10110 commits behind head on trunk.

Files with missing lines Patch % Lines
...Plugin/Command/Lifecycle/PluginFileHashService.php 0.00% 49 Missing ⚠️
...n/Command/Lifecycle/PluginChecksumCheckCommand.php 0.00% 44 Missing ⚠️
.../Command/Lifecycle/PluginChecksumCreateCommand.php 0.00% 25 Missing ⚠️
...amework/Store/Services/ExtensionChecksumLoader.php 0.00% 15 Missing ⚠️
...in/Command/Lifecycle/PluginChecksumCheckResult.php 0.00% 6 Missing ⚠️
...Framework/Store/Services/ExtensionDataProvider.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            trunk    #5362       +/-   ##
===========================================
- Coverage   65.46%   43.83%   -21.64%     
===========================================
  Files        3677     3081      -596     
  Lines       82789    87693     +4904     
===========================================
- Hits        54201    38439    -15762     
- Misses      28588    49254    +20666     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@M-arcus M-arcus changed the title NEXT-0000: Add functionality for plugin checksums NEXT-00000: Add functionality for plugin checksums Nov 22, 2024
@patzick patzick changed the title NEXT-00000: Add functionality for plugin checksums feat: add functionality for plugin checksums Dec 19, 2024
@AydinHassan
Copy link
Contributor

Hey @M-arcus, what is the state of this PR? Are you currently working on it?

In general we think this feature would be best served as an extension and not in the core framework, so if you do plan on continuing to work on it, please ping so we can discuss. We appreciate the effort and wouldn't want to disappoint any one involved, so we should communicate and make sure we are all on the same page! That does not mean we say no: but we should definitely discuss and weigh up the advantages/disadvantages of this living in the framework 😄

@M-arcus
Copy link
Contributor Author

M-arcus commented Jan 14, 2025

@AydinHassan

what is the state of this PR?

It is technically complete, but needs tests and admin components.

Are you currently working on it?

Yes, I am still working on this, but wasn't able to finish it due to time constraints and workload.

In general we think this feature would be best served as an extension and not in the core framework

What speaks against including it as a core feature? It was a core feature in Shopware 5 and helped integrators and agencies a lot when identifying modifications on the code.

@AydinHassan
Copy link
Contributor

@M-arcus in general for us, the question is rather: why does it need to be in core?

A few points we discussed:

  • We will have to maintain this feature that we did not build. You and the others involved are better placed to maintain it, you are experienced in the area and understand the problem and solution space. You can argue it's a simple feature but we are a small team and Shopware has many thousands of simple features and of course many complex ones.
  • It's only useful for a small subset of our users/customers
  • You are restricted to the platform release schedule

We don't doubt it's absolutely useful, but we're not convinced it should be in core. From our perspective it would be better as an extension. And not just better for us in terms of maintenance load, it's better for you and the ecosystem because you can iterate as you please.

Is there something special you cannot do as an extension?

@M-arcus
Copy link
Contributor Author

M-arcus commented Jan 17, 2025

@AydinHassan
In my opinion the best place for this functionality is within the core because it would provide a quick way to diagnose non-standard code changes, but I agree, that the target audience is rather small, since it would only affect plugin creators debugging during support requests.

I will move the code from this PR to a PR in https://github.com/FriendsOfShopware/FroshTools as a similar functionality already exists there, but only for changes in the core.

@AydinHassan
Copy link
Contributor

@M-arcus thanks for understanding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/core pr/declined Shopware declined the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants