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

Fix broken API output for composer based installs #13272

Merged

Conversation

mollux
Copy link
Contributor

@mollux mollux commented Jan 27, 2024

Q A
Bug fix? (use the a.b branch) [x]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #13243

Description:

(see also the research of @mallezie in #13243 (comment))

On the 5.0 branch (and all other 5 branches), the jms/serializer-bundle dependency is required as ^5.0 and locked to 5.1.0
However, when installing Mautic via composer (e.g. via the mautic/recommended-project), or testing/using the new Docker images (that internally use the mautic/recommended-project), there is no composer.lock file, and at this point 5.4.0 is used.

This is problematic, as there is has been (IMO a BC breaking) incompatibility in following versions:

The main change is that jms_serializer.metadata.annotation_driver is removed and replaced by jms_serializer.metadata.annotation_or_attribute_driver.
Since Mautic depends on that name, this currently breaks all API where data is processed by the jms/serializer-bundle

This PR addresses this, by locking the version to ^5.4, and adapting the logic in the compilerpass and alternatice class.

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. test if you can create a contact via the API and the correct data is returned.
  3. test if you can fetch that contact via the API and the correct data is returned.
  4. test is you can list all contacts via the API and the correct data is returned.

@mollux mollux added this to the 5.0.3 milestone Jan 27, 2024
@mollux mollux added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test API Anything related to the API labels Jan 27, 2024
@mollux
Copy link
Contributor Author

mollux commented Jan 27, 2024

I'm also running the tests on the API library to see if they pass, which is required.
See https://github.com/mautic/api-library/actions/runs/7678816774, which ran successful

Copy link

codecov bot commented Jan 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (527d684) 58.60% compared to head (c898f59) 58.60%.
Report is 3 commits behind head on 5.0.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##                5.0   #13272   +/-   ##
=========================================
  Coverage     58.60%   58.60%           
  Complexity    32985    32985           
=========================================
  Files          2183     2183           
  Lines         98755    98755           
=========================================
  Hits          57875    57875           
  Misses        40880    40880           
Files Coverage Δ
...le/DependencyInjection/Compiler/SerializerPass.php 100.00% <100.00%> (ø)
.../ApiBundle/Serializer/Driver/ApiMetadataDriver.php 75.75% <ø> (ø)

@mallezie
Copy link
Contributor

Tested the patch manually.

Default mautic install (from repo). Contact api provides expected results.
Mautic install from composer (with updated jms/ packages). Contact api does not give full results.
Mautic install from composer (with updated jms/packages) and with this PR applied. Gives back correct results.

Checking code, the changes look correct to me. Also can confirm the API tests pass.

One thing to note (perhaps not blocking on this thing), is if we could / should update tests to run on a composer based install (without the composer.lock file), or a seperate action running tests both in composer.lock install / composer based install. If not blocking i this i can create an issue for that?

Further more (noting i'm not up to speed with the API-tests) if is we could see if this has actually test coverage in the api-tests package. I think this would need an API (failing) test run with only the package update.

Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, Mattias!

@escopecz
Copy link
Sponsor Member

@mallezie can you please validate if your tests of this PR were successful? I'm not clear on that from your comment. Especially this line is confusing to me:

Mautic install from composer (with updated jms/ packages). Contact api does not give full results.

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged code-review-passed PRs which have passed code review and removed ready-to-test PR's that are ready to test labels Jan 29, 2024
@mallezie
Copy link
Contributor

@escopecz
My tests were indeed successfull.

Mautic install from composer (with updated jms/ packages). Contact api does not give full results.

This just was to confirm the issue that on composer based install (without the changes from the PR) the issue occurs. With the changes in this PR. Everything is okay.

@escopecz escopecz added ready-to-test PR's that are ready to test composer Any bugs or PRs relating to composer and removed pending-test-confirmation PR's that require one test before they can be merged labels Jan 29, 2024
@escopecz escopecz merged commit 4701e43 into mautic:5.0 Jan 29, 2024
14 checks passed
@mollux mollux deleted the bugfix/compatibility-with-jms-serializer-bundle branch January 29, 2024 15:24
@mallezie
Copy link
Contributor

For future developments / problems in this area. I've verified that the API tests would have caught this issue (if they ran a composer based install). Which was an open concern from my side.

Since the api library tests are switching to a docker based (composer based install) for their testing, there is no point now in adjusting the api-library tests. If fields are not shown in an api-response, the api tests catch it, so that's good.

@mollux mollux added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed ready-to-test PR's that are ready to test labels Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Anything related to the API bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review composer Any bugs or PRs relating to composer ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Creating/getting a contact returns empty contact object
3 participants