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.

Sign up for GitHub

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

Jump to bottom

[Improvement]: Move grid data related functions to admin-ui-classic-bundle #16745

Draft
wants to merge 15 commits into
base: 11.x
Choose a base branch
from
Draft

[Improvement]: Move grid data related functions to admin-ui-classic-bundle #16745

wants to merge 15 commits into from

Conversation

kingjia90
Copy link
Contributor

Changes in this pull request

Resolves #15971
Requires pimcore/admin-ui-classic-bundle#457

@kingjia90 kingjia90 added this to the 11.2.0 milestone Mar 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 6, 2024

Review Checklist

  • Target branch (11.1 for bug fixes, others 11.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

@kingjia90 kingjia90 marked this pull request as draft March 6, 2024 12:51
@kingjia90
Copy link
Contributor Author

kingjia90 commented Mar 6, 2024 •
edited

The combination pimcore ^11.2 and admin ui ^1.4 seem it would work fine (test green) as pimcore/admin-ui-classic-bundle@27f998c
Just a little tricky to test on pimcore/pimcore as there are no hard requirement for which version of admin ui it should use, outside the platform version

kingjia90
kingjia90 commented Mar 6, 2024
View reviewed changes
models/DataObject/Service.php
*
*
*
* @deprecated Despite being internal, keeping in here to avoid bundles to require 11.2 or set a compatibility layer when bundles support both 10 & 11
Copy link
Contributor Author

@kingjia90 kingjia90 Mar 6, 2024 •
edited

Choose a reason for hiding this comment

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

After a second scan, there are no bundles that support both 10 and 11 that would be affected, so either keep it as is, or bump the minimum requirement to 11.2 in the following bundles, or compatibility layer with class_exist on bundle side

@blankse
Copy link
Contributor

blankse commented Mar 6, 2024 •
edited

@kingjia90 So it is not possible to use pimcore 11.2 with admin ui < 1.4? So I think you should add it to the conflict section in the composer.json

@kingjia90
Copy link
Contributor Author

@blankse that's a nice idea, thank you!

@blankse
Copy link
Contributor

blankse commented Mar 6, 2024

Maybe we should also add the admin ui to the require-dev section to fix the phpstan tests? There are also many errors in the baseline because the bundle is missing.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 7, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@fashxp fashxp modified the milestones: 11.2.0, 11.3.0 Mar 11, 2024
@markus-moser markus-moser self-assigned this Apr 26, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Apr 29, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiklasBr NiklasBr NiklasBr left review comments

Assignees

@markus-moser markus-moser

Labels
Improvement Pimcore:Priority Pimcore:ToDo
Projects
None yet
Milestone
11.3.0
Development

Successfully merging this pull request may close these issues.

Move function to admin-ui-classic-bundle
6 participants
@kingjia90 @blankse @NiklasBr @markus-moser @fashxp @wisconaut