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

Allow easier extension of the timezone guessing #548

Merged
merged 2 commits into from Nov 15, 2021

Conversation

heiglandreas
Copy link
Contributor

@heiglandreas heiglandreas commented Oct 29, 2021

This will ease customization of timezone-guessing as it is now gets easier to extend that process with own implementations (as long as they implement the appropriate interface)

This is espechially necessary when wanting to actually guess a timezone via the rules defined in the VTIMEZONE-entry (which is currently not done)

This change does not introduce any new logic. It just rearranges the already existing code in a different way to allow creating own TimezoneGuessers that can extend the functionality like this:

class BuesingenGuesser implements TimezoneGuesser
{
    public function guess(VTimeZone $vtimezone, bool $throwWhenUnsure = false): ?DateTimeZone
    {
        return new DateTimeZone('Europe/Busingen');
    }
}
TimezoneUtil::addTimeZoneGuesser(new BuesingenGuesser());

Now every time a timezone is not resolvable the timezone will be resolved to Europe/Busingen instead of UTC (or whatever else is set via date_default_timezone_set())

Copy link
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

Hey Andreas,

Really like this direction. Do we have/need test coverage for this code?

lib/TimeZoneUtil.php Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 30, 2021

Codecov Report

Merging #548 (ff902c6) into master (a7460c5) will decrease coverage by 0.35%.
The diff coverage is 93.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #548      +/-   ##
============================================
- Coverage     98.69%   98.34%   -0.36%     
- Complexity     1818     1848      +30     
============================================
  Files            66       71       +5     
  Lines          4454     4521      +67     
============================================
+ Hits           4396     4446      +50     
- Misses           58       75      +17     
Impacted Files Coverage Δ
lib/TimeZoneUtil.php 69.64% <86.66%> (-28.09%) ⬇️
lib/TimezoneGuesser/FindFromTimezoneIdentifier.php 92.85% <92.85%> (ø)
lib/TimezoneGuesser/FindFromOffset.php 100.00% <100.00%> (ø)
lib/TimezoneGuesser/FindFromTimezoneMap.php 100.00% <100.00%> (ø)
lib/TimezoneGuesser/GuessFromLicEntry.php 100.00% <100.00%> (ø)
lib/TimezoneGuesser/GuessFromMsTzId.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7460c5...ff902c6. Read the comment docs.

@heiglandreas heiglandreas force-pushed the allowMoreTimezoneGuesser branch 2 times, most recently from 0cd54e2 to c5f2bec Compare November 2, 2021 09:02
@heiglandreas
Copy link
Contributor Author

Should be good by now..

Copy link
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

just leaving a few comments. but lets wait what the other devs think.

*
* Source: http://msdn.microsoft.com/en-us/library/aa563018(loband).aspx
*/
public static $microsoftExchangeMap = [
Copy link
Member

@staabm staabm Nov 2, 2021

Choose a reason for hiding this comment

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

I am not sure we can drop this field, as other code might rely on it beeing defined.

there is more public api changed with this PR in a non-BC manner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right!. I'll readd those other public items to stay backwards compatible.

Main question though is whether I shall do it in a way that allows them to be easily removed at a later stage (meaning to duplicate some code) or to have as little duplicate code as possible.

Personally I'd opt for the former version but am happy to modify it to the later one.

lib/TimeZoneUtil.php Show resolved Hide resolved
lib/TimeZoneUtil.php Show resolved Hide resolved
@phil-davis
Copy link
Contributor

@heiglandreas the code style is failing. It is easily fixed by:

composer cs-fixer

and then commit and push the changes it makes.

@phil-davis
Copy link
Contributor

https://github.com/sabre-io/vobject/runs/4101303777?check_suite_focus=true
Class Sabre\VObject\TimeZoneUtil referenced with incorrect case

The code analysis is fussy. Please fix that up.

Note: I released https://github.com/sabre-io/vobject/releases/tag/4.3.6 an hour ago. But when we get this PR sorted out, it will be easy to make a 4.3.7 release.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

Just a couple of tiny comments.

tests/VObject/TimeZoneUtilTest.php Outdated Show resolved Hide resolved
lib/TimeZoneUtil.php Outdated Show resolved Hide resolved
@phil-davis
Copy link
Contributor

@staabm @evert @DeepDiver1975 please review and comment.
This would be at least a minor version bump (new feature - way of processing timezones). Please think if there is any remaining BC break that would require a major version.

Copy link
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

lgtm

This will ease customization of timezone-guessing as it is now gets easier
to extend that process with own implementations (as long as they
implement the appropriate interface)

This is espechially necessary when wanting to actually guess a timezone
via the rules defined in the VTIMEZONE-entry (which is currently not
done)
@phil-davis
Copy link
Contributor

I squashed and rebased to get up-to-date clean CI.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM. There is some discussion in PR #553 about tests to cover the functions addTimezoneGuesser and addTimezoneFinder but that can be left until later.

@phil-davis phil-davis merged commit 9157772 into sabre-io:master Nov 15, 2021
@phil-davis phil-davis mentioned this pull request Nov 15, 2021
@heiglandreas heiglandreas deleted the allowMoreTimezoneGuesser branch November 16, 2021 06:12
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