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

Support 7 #450

Merged
merged 12 commits into from
Feb 17, 2024
Merged

Support 7 #450

merged 12 commits into from
Feb 17, 2024

Conversation

bartmcleod
Copy link
Collaborator

  • Supports only SF 7
  • Fixes UI issues that were probably introduced with using the newer Angular version, or changes in a dependency. Http responses are now promises and must be handled as such. A couple of fixes were needed, this might introduce new UI bugs, but from what I could test manually I think it is looking good and working.

@k0d3r1s
Copy link

k0d3r1s commented Jan 5, 2024

why use newer angular on any change in ui withing upgrade to symfony 7? can those things be split in 2 separate PRs?

@bartmcleod
Copy link
Collaborator Author

@k0d3r1s Yes, splitting that would be the "normal" procedure. I am not even sure the bug was introduced by the new Angular version. On second thought, I am pretty sure I tested that to work just fine, but it is quite some time ago and a cached script version may have been observed at the time: I cannot be sure anymore. What I need to do now is just make it work and everything that needs to be done to make it work will be in the same PR. This PR is also not complete. Although the UI and the commands all seem to work just fine, the unit tests fail, because Propel does not support Symfony 7. So I'm currently updating this to remove all Propel related code.

@bartmcleod bartmcleod marked this pull request as draft January 5, 2024 19:57
@k0d3r1s
Copy link

k0d3r1s commented Jan 14, 2024

@bartmcleod hey, do you have any ETA here?

@gilles-g
Copy link
Member

@bartmcleod
I'm ok too to remove Propel, and maybe one day why not mongo.

@bartmcleod
Copy link
Collaborator Author

@k0d3r1s I have some time for it today. I am stuck at the unit tests: a new XmlFileDriver is in use that does not accept the old settings, but also gives no suggestions on what correct settings could be.

@Seb33300
Copy link
Contributor

Seb33300 commented Jan 15, 2024

You should create separate pull requests to fix UI issues and Symfony upgrade if they are unrelated.
This will make the review easier and will allow maintainers to merge them faster.

@k0d3r1s
Copy link

k0d3r1s commented Feb 16, 2024

@bartmcleod hey, any updates here?

@bartmcleod
Copy link
Collaborator Author

bartmcleod commented Feb 16, 2024

@k0d3r1s I have looked at it several times, but I can't understand how the underlying component is failing. I mean I understand the original component is no longer there, and I only assumed that the one I'm using now would be the correct replacement, but it obviously isn't and I can't find documentation on what would be the correct replacement.

@bartmcleod bartmcleod marked this pull request as ready for review February 17, 2024 10:16
@bartmcleod
Copy link
Collaborator Author

@k0d3r1s I managed to fix the unit tests on my machine. Now I need to figure out why the build still fails on GitHub.

@bartmcleod
Copy link
Collaborator Author

bartmcleod commented Feb 17, 2024

@k0d3r1s The build fails because it somehow uses node 16 actions and GitHub has evolved to use node 20 actions. So nothing to do with the code itself, but I need to fix the action before the build will pass

@bartmcleod
Copy link
Collaborator Author

Now that I have passing tests I will merge it, because I tested the UI and commands already when the code itself was fixed. This means that SF 6 users will have to target the 6.0 branch instead of master from now on.

@bartmcleod bartmcleod merged commit 7c8531f into lexik:master Feb 17, 2024
1 check passed
@bartmcleod bartmcleod deleted the support-7 branch February 17, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants