-
-
Notifications
You must be signed in to change notification settings - Fork 818
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.3] [Deprecations] Switch YamlUpdater to use Bolt filesystem for YAML handling #6557
[3.3] [Deprecations] Switch YamlUpdater to use Bolt filesystem for YAML handling #6557
Conversation
Side note: $ ./app/nut config:get thumbnails/aliases/myimageformat/cropping -f theme://theme.yml
thumbnails/aliases/myimageformat/cropping: crop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can ConfigGet/Set be abstracted? The majority of the lines in both of those files are the same.
src/Configuration/YamlUpdater.php
Outdated
$this->parsed = $this->parser->parse($yaml, true, true); | ||
if (is_string($file)) { | ||
$fs = $app['filesystem']->getFilesystem('config'); | ||
$file = $fs->getFile($file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not:
$file = $app['filesystem']->getFile('config://' . $file);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw … exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nvm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/Configuration/YamlUpdater.php
Outdated
@@ -156,7 +149,7 @@ protected function verify() | |||
} | |||
|
|||
// This will throw a ParseException If the YAML is not valid | |||
$this->parser->parse($this->yaml, true, true); | |||
$this->parsed = $this->file->parse(['exceptionsOnInvalidType' => true, 'objectSupport' => true]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work as the file doesn't have the new contents yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Yeah, I did wonder … I might in the morning |
Done |
This should be GTG now, right? |
/me nudges @CarsonF |
src/Configuration/YamlUpdater.php
Outdated
{ | ||
$pattern = str_replace('/', ':.*?', $key); | ||
preg_match_all('/^' . $pattern . '(:\s*)/mis', $this->file->read(), $matches, PREG_OFFSET_CAPTURE); | ||
preg_match_all('/^' . $pattern . '(:\s*)/mis', $this->lines->join("\n"), $matches, PREG_OFFSET_CAPTURE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected 1 space after comma in function call; 2 found
src/Configuration/YamlUpdater.php
Outdated
$yaml = $this->lines->join("\n"); | ||
|
||
$pattern = '/^' . str_replace('/', ':.*?', $key) . '(:\s*)/mis'; | ||
preg_match($pattern, $yaml, $matches, PREG_OFFSET_CAPTURE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected 1 space after comma in function call; 2 found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GawainLynch If you are good with my changes feel free to merge
Cuts out deprecations in SF 3.1+ too