inpsyde / vip-composer-plugin

A Composer plugin to ease deployment to wordpress.com VIP servers alongside Composer-based local development.
MIT License
11 stars 0 forks source link

SymlinkVipGoDir - various fixes and improvments on run method #5

Closed Chrico closed 3 years ago

Chrico commented 3 years ago

This pull request contains various improvments on the SymlinkVipGoDir:

Remove is_dir-check which is not save enough to rely on for symlinked directories.

The Filesystem::removeDirectory($directory) shouldn't only be used when is_dir() returns true. In some cases, like symlinks in Apache which are not configured correctly, this function will return false, eventho there is a directory. To have a clean run on every composer vip --local we need to ensure, that symlinks are set correctly.

It is saver to use this method always. It has internally all checks for is_link , is_dir and even isJunction() to support Windows NTFS Junction.

Fix missing assigment for normalizePath on $link.

In run-method, the Filesystem::normalizePath($link) was called, but the return value was never assigned back to $link. In the end, the call did nothing change.

Add note for failing relativeSymlink.

Since i had a really hard time to figure out what failed (Symlinks were not created because of is_dir() and missing Filesystem::removeDirectory($directory)), it would be nice to have in future at least for failing parts, like symlinks, a bit more context about. Sadly `Filesystem::relativeSymlink($target, $link) changes internally those values to work with, but does not expose anything to outside.

Creating of symlinks after switching a system - but keeping old code - failed silently and broke the site.

Switching between different systems now works smoother, because we can ensure that symlinks/junctions are created correctly on each run.

No.

Happy weekend! ☕ 🍷

gmazzap commented 3 years ago

We might should consider not catching Exceptions inside every Task and let them bubble up to the Runner, so that he can take care of it.

It's already like that. The command takes care of catching all the exceptions and fail if something happen. See https://github.com/inpsyde/vip-composer-plugin/blob/improve_symlink_vip_go_dir/src/Command.php#L179-L185

So everytime there's an exception in any task, the exception message is output and the command terminates with a non-zero exit code.

We can merge as-is, and let the exception bubble-up if needed, but if we do it, then you could see something like:

$from (x/z) and $to (x/y) must be absolute paths.

without any more context. If we catch the exception at task level, we could show something like:

Failed creating relative symlink for "x/z" to "x/y" $from (x/z) and $to (x/y) must be absolute paths.

So there's more information about what failed. So maybe we can catch for having context and then re-throw to let Command handle the overall failure.

I'm fine in merging like it is now, but if your idea was to get information on what failed, current status might not be enough.

Chrico commented 3 years ago

@gmazzap if its okay for you i would remove the Io::errorLine() output and open a new ticket to check through all Tasks if we can catch failures better by throwing Exceptions when something does fail, but is actually required. Okay for you?

gmazzap commented 3 years ago

@Chrico ok pleaase go on.

Chrico commented 3 years ago

@gmazzap removed the error output and opened an issue https://github.com/inpsyde/vip-composer-plugin/issues/6.

Feel free to merge! Thx 👍