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

[3.4] Implement use of bolt/common #6892

Merged
merged 8 commits into from Aug 19, 2017

Conversation

Projects
None yet
2 participants
@GawainLynch
Contributor

GawainLynch commented Aug 12, 2017

No description provided.

GawainLynch added some commits Aug 11, 2017

@GawainLynch GawainLynch added this to the Bolt 3.4 - Feature release milestone Aug 12, 2017

@GawainLynch GawainLynch requested a review from CarsonF Aug 12, 2017

@CarsonF

Good start :)

  • There's lots of places Json::parse() is called with a second parameter that is not needed.
  • There are places Json::dump() is called with 0, but probably could be fine with some options. I understand some places don't make sense for pretty print, but I don't see why we need to escape slashes or unicode.
  • Str has those two methods that are BC breaks. I think it might be better to trigger deprecation warnings for each method call instead of the file. That was it is easier to find usages that need to be fixed.
@@ -45,7 +46,7 @@ public function __construct($config = null)
$this->config = $config;
$this->magicQuotes = get_magic_quotes_gpc();
$this->safeMode = ini_get('safe_mode');

This comment has been minimized.

@CarsonF

CarsonF Aug 15, 2017

Member

You can just hardcode false here. safe_mode was removed in PHP 5.4.

@CarsonF

CarsonF Aug 15, 2017

Member

You can just hardcode false here. safe_mode was removed in PHP 5.4.

This comment has been minimized.

@GawainLynch

GawainLynch Aug 15, 2017

Contributor

Typo <insert facepalm emoji>

@GawainLynch

GawainLynch Aug 15, 2017

Contributor

Typo <insert facepalm emoji>

This comment has been minimized.

@GawainLynch

GawainLynch Aug 15, 2017

Contributor

Change is below … caught me out for a minute why this wasn't refreshing

@GawainLynch

GawainLynch Aug 15, 2017

Contributor

Change is below … caught me out for a minute why this wasn't refreshing

Show outdated Hide outdated src/Provider/DebugServiceProvider.php
Show outdated Hide outdated src/Storage/Entity/LogChange.php
// This handles an inconsistency in the result from the JSON parser across 5.x and 7.x of PHP
if ($value === '') {
return false;
}

This comment has been minimized.

@CarsonF

CarsonF Aug 15, 2017

Member

I don't think Json::test includes this check, maybe we need to add it?

@CarsonF

CarsonF Aug 15, 2017

Member

I don't think Json::test includes this check, maybe we need to add it?

This comment has been minimized.

@GawainLynch

GawainLynch Aug 15, 2017

Contributor

Ah, good point!

@GawainLynch

GawainLynch Aug 15, 2017

Contributor

Ah, good point!

This comment has been minimized.

@GawainLynch

GawainLynch Aug 16, 2017

Contributor

I'll update the constraint in this PR when we tag that branch on common 👍

@GawainLynch

GawainLynch Aug 16, 2017

Contributor

I'll update the constraint in this PR when we tag that branch on common 👍

This comment has been minimized.

@GawainLynch

GawainLynch Aug 18, 2017

Contributor

This is included now in bolt/common 1.1.0+

@GawainLynch

GawainLynch Aug 18, 2017

Contributor

This is included now in bolt/common 1.1.0+

Show outdated Hide outdated tests/phpunit/unit/Controller/Backend/ExtendTest.php
Show outdated Hide outdated src/Helpers/Str.php
@GawainLynch

This comment has been minimized.

Show comment
Hide comment
@GawainLynch

GawainLynch Aug 15, 2017

Contributor

There's lots of places Json::parse() is called with a second parameter that is not needed.

There are places Json::dump() is called with 0, but probably could be fine with some options. I understand some places don't make sense for pretty print, but I don't see why we need to escape slashes or unicode.

I knew this would come up, was waiting to see what/how 😆 Basically BC on the pretty print, and I would've expected the same on the output escaping, no?

Contributor

GawainLynch commented Aug 15, 2017

There's lots of places Json::parse() is called with a second parameter that is not needed.

There are places Json::dump() is called with 0, but probably could be fine with some options. I understand some places don't make sense for pretty print, but I don't see why we need to escape slashes or unicode.

I knew this would come up, was waiting to see what/how 😆 Basically BC on the pretty print, and I would've expected the same on the output escaping, no?

@CarsonF

This comment has been minimized.

Show comment
Hide comment
@CarsonF

CarsonF Aug 15, 2017

Member

I knew this would come up, was waiting to see what/how 😆 Basically BC on the pretty print, and I would've expected the same on the output escaping, no?

I mean JSON shouldn't be programmatically inspected, it should just be parsed. The only exception, I think, would be test assertions. So regardless of the dump flags, the output should be valid JSON which can be parsed anywhere. Now with that said, I know pretty printed can make things less readable for small data and increases size which should only matter when it is being transported over the wire. HTML might have a problem with unescaped unicode, that should be double checked...I'm still drinking ☕️ . I've never seen the unescaped slashes hurt anything either.

Member

CarsonF commented Aug 15, 2017

I knew this would come up, was waiting to see what/how 😆 Basically BC on the pretty print, and I would've expected the same on the output escaping, no?

I mean JSON shouldn't be programmatically inspected, it should just be parsed. The only exception, I think, would be test assertions. So regardless of the dump flags, the output should be valid JSON which can be parsed anywhere. Now with that said, I know pretty printed can make things less readable for small data and increases size which should only matter when it is being transported over the wire. HTML might have a problem with unescaped unicode, that should be double checked...I'm still drinking ☕️ . I've never seen the unescaped slashes hurt anything either.

@GawainLynch

This comment has been minimized.

Show comment
Hide comment
@GawainLynch

GawainLynch Aug 18, 2017

Contributor

OK! This should be good to go, and our combined hashing out of the JSON parameters should have both our O.C.D. triggers covered 👍

Unless you have derps to point out, lets :shipit: as by the time you're online today I'll be about to go offline, and we lose you over the weekend. I want to tie up the last of the beta work over the weekend & Monday EU time as @bobdenotter will be back as well.

Contributor

GawainLynch commented Aug 18, 2017

OK! This should be good to go, and our combined hashing out of the JSON parameters should have both our O.C.D. triggers covered 👍

Unless you have derps to point out, lets :shipit: as by the time you're online today I'll be about to go offline, and we lose you over the weekend. I want to tie up the last of the beta work over the weekend & Monday EU time as @bobdenotter will be back as well.

@CarsonF

Thanks dood!

@GawainLynch GawainLynch merged commit d26a516 into bolt:3.4 Aug 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@GawainLynch GawainLynch deleted the GawainLynch:hotfix/bolt-common branch Aug 19, 2017

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