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 sabre/xml v4 #607

Merged
merged 6 commits into from Jan 22, 2023

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jan 22, 2023

I accidentally released 4.5.2 on Friday with all the code in master branch. That code has various PHP 7.4-only things that will not work properly on PHP 7.1 7.2 7.3 etc.

See comment #588 (comment) which reports this mistake.

This PR:

  1. backports the "Support sabre/xml v4" change to 4.5 branch
  2. backports "Skip GMT+n timezone test cases on PHP 8.1.14 and 8.2.1" so that unit tests can be green
  3. adds return types to things that implement xmlSerialize
  4. adds return types to things that implement xmlDeserialize
  5. backports "Prepare 4.5.2" changelog and Version.php change
  6. adds changelog and version bump to 4.5.3

If I merge this and then release 4.5.3 from the 4.5 branch, I think that it should be fine.

The added return types are all supported from PHP 7.1 upward, so that is nice.

The only thought that I have now is that if consumers have implemented child classes that redeclare xmlSerialize or xmlDeserialize then maybe they will also have to add a return type void or array ? And so that will mean that the release is not strictly backward-compatible. And so a patch release is not correct.

Thoughts please @staabm @DeepDiver1975 or anyone.

@codecov
Copy link

codecov bot commented Jan 22, 2023

Codecov Report

Merging #607 (bad63c0) into 4.5 (167a599) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                4.5     #607      +/-   ##
============================================
+ Coverage     98.35%   98.57%   +0.22%     
  Complexity     1897     1897              
============================================
  Files            71       71              
  Lines          4548     5483     +935     
============================================
+ Hits           4473     5405     +932     
- Misses           75       78       +3     
Impacted Files Coverage Δ
lib/Node.php 100.00% <ø> (ø)
lib/Component.php 99.19% <100.00%> (+0.03%) ⬆️
lib/Component/VCard.php 100.00% <100.00%> (ø)
lib/Parameter.php 99.28% <100.00%> (+0.04%) ⬆️
lib/Parser/XML/Element/KeyValue.php 84.61% <100.00%> (ø)
lib/Property.php 99.05% <100.00%> (+0.06%) ⬆️
lib/Property/Boolean.php 47.05% <0.00%> (-2.95%) ⬇️
lib/TimeZoneUtil.php 68.42% <0.00%> (-1.23%) ⬇️
lib/Component/VCalendar.php 94.64% <0.00%> (-0.10%) ⬇️
lib/UUIDUtil.php 100.00% <0.00%> (ø)
... and 40 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@phil-davis phil-davis marked this pull request as ready for review January 22, 2023 05:02
@staabm
Copy link
Member

staabm commented Jan 22, 2023

Alternatively I think we could release 4.5.1 as 4.5.3 again.

Then we could use 4.5.2 and publish it as a new major.

@staabm
Copy link
Member

staabm commented Jan 22, 2023

The changes here look fine if you prefer it

@phil-davis phil-davis force-pushed the support-sabre-xml-v4-4.5-series branch from 9a857cd to d672f6c Compare January 22, 2023 09:12
@phil-davis phil-davis force-pushed the support-sabre-xml-v4-4.5-series branch from d672f6c to bad63c0 Compare January 22, 2023 09:32
@phil-davis
Copy link
Contributor Author

The changes here look fine if you prefer it

Do you think that the void and array return type declarations are a "significant" backward-compatibility problem?

If I just re=release 4.5.1 as 4.5.3 then the ability to use sabre/xml v3 or v4 (PR #603 ) get to happen.

If I do this PR, then @gharlan (and others) can have the latest sabre/xml now.

@staabm
Copy link
Member

staabm commented Jan 22, 2023

I don't have a strong opinion which way to go from here

@phil-davis phil-davis merged commit fe6d918 into sabre-io:4.5 Jan 22, 2023
@phil-davis phil-davis deleted the support-sabre-xml-v4-4.5-series branch January 22, 2023 12:22
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.

None yet

3 participants