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

PHP: ensure 5.3 compatibility #271

Merged
merged 1 commit into from
Jul 15, 2015
Merged

Conversation

virtualtam
Copy link
Member

Relates to #250

Modifications

  • add PHP 5.3 to Travis environments
  • rewrite array declarations: explicitely use array() instead of []
  • move checkPHPVersion to application/Utils.php
  • move timezone functions to application/TimeZone.php
  • improve test coverage
  • bump required version from 5.1.0 to 5.3.x

@ArthurHoaro
Copy link
Member

You forgot L949. But other than that everything seems to be working with PHP 5.3.3.

@virtualtam
Copy link
Member Author

Actually, I think a couple methods can also be simplified and covered by tests:

  • isTZvalid()
  • templateTZform()

Some comments are outdated, too, there are references to 5.1.x compatibility

Working on it! :)

@virtualtam virtualtam force-pushed the php53 branch 3 times, most recently from 7a02c95 to e3b48b3 Compare July 11, 2015 12:53
@virtualtam virtualtam added cleanup code cleanup and refactoring and removed in progress labels Jul 11, 2015
@virtualtam
Copy link
Member Author

@ArthurHoaro I've done some additional edits and cleanup, could you confirm it works with a 5.3.x version?

$continents_html.='<option value="'.$continent.'"'.($pcontinent==$continent?' selected':'').'>'.$continent.'</option>';
}

$cities_html = $cities[$pcontinent];
Copy link
Member

Choose a reason for hiding this comment

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

$citites_html is never used.

Copy link
Member Author

Choose a reason for hiding this comment

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

kenavo, $cities_html :)

@ArthurHoaro
Copy link
Member

Aside from my minor comments, everything seems to be working with PHP 5.3.3 and unit tests are OK.

Great job!

@virtualtam
Copy link
Member Author

Thanks for reviewing!
I'll see how the TZ methods can be improved (e.g. not only coding style) and write a bit more clever tests :)

@ArthurHoaro
Copy link
Member

Perfect. Also, remind to update README.

@virtualtam virtualtam force-pushed the php53 branch 2 times, most recently from 6bc288e to 1464542 Compare July 12, 2015 17:40
@virtualtam
Copy link
Member Author

Comments taken into account, here are the more significant changes since the last proposed PR:

  • update README
  • refactor TZ code for readability, more relevant docstrings and inline comments
  • more in-depth unit tests: check generated HTML and Javascript content
  • [EDIT] rebased :)

@virtualtam virtualtam force-pushed the php53 branch 2 times, most recently from 7ab6c25 to 12df6c6 Compare July 12, 2015 19:05
}

$tz = $continent.'/'.$city;
if (in_array($tz, timezone_identifiers_list())) {
Copy link
Member

Choose a reason for hiding this comment

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

That's a bit picky, but you can just return in_array.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, I'll re-submit :)

@ArthurHoaro
Copy link
Member

Great job! I'll make a final test in 5.3 and we can merge this. :)

Relates to shaarli#250

Modifications
 - supported version
   - bump required version from 5.1.0 to 5.3.x
   - update README
   - add PHP 5.3 to Travis environments
 - rewrite array declarations: explicitely use array() instead of []
 - move checkPHPVersion to application/Utils.php
 - move timezone functions to application/TimeZone.php
   - cleanup code
   - improve test coverage

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
ArthurHoaro added a commit that referenced this pull request Jul 15, 2015
PHP: ensure 5.3 compatibility
@ArthurHoaro ArthurHoaro merged commit 874f858 into shaarli:master Jul 15, 2015
@virtualtam
Copy link
Member Author

Thanks for testing :)

@virtualtam virtualtam deleted the php53 branch July 17, 2015 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup code cleanup and refactoring enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants