Use a unique images/icons directory#100
Conversation
rxu
left a comment
rxu
left a comment
There was a problem hiding this comment.
To be honest, looking at the PR now I think we're performing some risky and unreliable migration actions for a little advantage :) Maybe leave it as is now?
| private const NEW_ICON_DIR = 'images/site_icons'; | ||
| private const OLD_ICON_DIR = 'images/icons'; |
There was a problem hiding this comment.
Probably just add OLD_ICON_DIR to the ext class and then use both it and PWA_ICON_DIR here statically.
There was a problem hiding this comment.
In the migration I wanted to keep as much as possible self contained. OLD_ICON_DIR is only ever used here so there's no real reason to put it into the ext file. I thought about using PWA_ICON_DIR here for NEW_ICON_DIR, but then the migration would rely on our ext file which I want to avoid. Also, setting a constant with another constant seems wonky too const NEW_ICON_DIR = PWA_ICON_DIR
|
|
||
| public function effectively_installed() | ||
| { | ||
| return $this->get_filesystem()->exists($this->container->getParameter('core.root_path') . self::NEW_ICON_DIR); |
There was a problem hiding this comment.
And also if pwa_icon_small && pwa_icon_large config values aren't empty, otherwise there's nothing to migrate.
There was a problem hiding this comment.
We still need to create the new site_icons directory regardless of if there is an existing config value
| return ['\phpbb\webpushnotifications\migrations\add_acp_pwa_configs']; | ||
| } | ||
|
|
||
| public function update_data(): array |
There was a problem hiding this comment.
Add revert_data()? Basically removing the icons dir.
There was a problem hiding this comment.
No we leave the directory behind. It could be used for other images, like favicons and additional touch icons. We never want to delete any files outside of the extension.
| $message = $copied ? 'LOG_WEBPUSH_ICON_COPY_SUCCESS' : 'LOG_WEBPUSH_ICON_DIR_SUCCESS'; | ||
| $result = [$message => [self::NEW_ICON_DIR]]; | ||
| } | ||
| catch (filesystem_exception $e) |
There was a problem hiding this comment.
Not sure how try/catch will work within migration. Feels like it'll throw uncaught exception on failure which will stop extension installation and break things on user end.
There was a problem hiding this comment.
That's why there's the catch. It'll gracefully catch any of the exceptions the filesystem throws and log the event. Doesn't stop installation.
|
| 'LOG_WEBPUSH_SUBSCRIPTION_REMOVED' => '<strong>Удалена подписка на браузерные push—уведомления:</strong>» %s', | ||
| 'LOG_WEBPUSH_MESSAGE_FAIL' => '<strong>Не удалось отправить браузерное push—уведомление:</strong><br>» %s', | ||
| 'LOG_WEBPUSH_SUBSCRIPTION_REMOVED' => '<strong>Удалена подписка на браузерные push—уведомления:</strong><br>» %s', | ||
| 'LOG_WEBPUSH_ICON_DIR_FAIL' => '<strong>Webpush Notifications could not migrate the following item in phpBB’s images directory:</strong><br>» %1$s » %2$s', |
Co-authored-by: rxu <rxu@mail.ru>
Co-authored-by: rxu <rxu@mail.ru>
2 participants
No description provided.