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

BUGFIX: Publish Resources On Windows Without Admin Rights #3106

Draft
wants to merge 1 commit into
base: 7.3
Choose a base branch
from

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jun 30, 2023

Tested on windows and works ^^

see https://discuss.neos.io/t/running-neos-flow-on-windows-10-success/2752 and especially thanks to @creative-resort for the hint https://discuss.neos.io/t/running-neos-flow-on-windows-10-success/2752/6?u=marc

Now you should be able to setup a Neos on Windows without the admin shell ^^

flow flow:cache:warmup should just work on a fresh install ^^

We currently only need to link whole directories, thats why the "hack" works.

previously it would fail to publish Neos.Flow\Resources\Public into Web\_Resources\Static\Packages\Neos.Flow

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

@dlubitz
Copy link
Contributor

dlubitz commented Jun 30, 2023

Uff, this is a tough one.
I can imagine this might open other symlink issues, which are not handled here.
And I don't the drawbacks of junctions in comparison to symlinks.
Also what about users, which want to use symlinks (with supporting environment)?

Actually, when I was using Windows for Neos development, the symlinks where a "one time" issue you had to solve/configure. Afterwards I never got in touch with it. Maybe we just need to improve documentation here. On the other hand, it would be great to get it working without any tweeks to the host system.

@dlubitz
Copy link
Contributor

dlubitz commented Jun 30, 2023

What about a dedicated FileSystemWindowsJunctionTarget?

@mhsdesign
Copy link
Member Author

i could also imagine first trying symlink ignoring all warnings and if this failed - only then try the windows special case. That would still allow me to use the windows admin shell to create "real" symlinks.

I just want to make flow work out of the box on windows, as many newcomers just start there.

@mhsdesign
Copy link
Member Author

image (2)

The new setup tool will now warn the windows user ;)

neos/neos-development-collection#4243

@mhsdesign
Copy link
Member Author

We should also ignore any errors inside symlink via @ so the whole flow booting doesnt fail on windows "just" because the static resource collections cant be published (a little intense ^^)

if ($identifier !== 'Flow_PublicResourcesFiles') {
return;
}
$objectManager = $bootstrap->getObjectManager();
$resourceManager = $objectManager->get(ResourceManager::class);
$resourceManager->getCollection(ResourceManager::DEFAULT_STATIC_COLLECTION_NAME)->publish();

static resources are always republished in development mode when the file monitor detects changes inside Flow_PublicResourcesFiles or when the cache is flushed.

So i think its fine to just log a warning that they coudnt be published, and the user has to either A: run publish in an admin shell, or properly configure the windows user.

C: would be to incorporate this fix, or fall back to copying files instead of symlinking but as you said @dlubitz it might be more of a one time configuration issue ;)

@mhsdesign mhsdesign marked this pull request as draft July 18, 2023 13:17
@kdambekalns
Copy link
Member

We should also ignore any errors inside symlink via @ so …

Generally speaking the whole DNA of Flow & Neos loathes the shut-up operator. Flow uses it 36 times in the whole codebase (still too many times), and Neos not at all. So, please only if there is no other way. 🙏

@mhsdesign
Copy link
Member Author

So, please only if there is no other way.

@kdambekalns yes i agree - its just - well this is the alternative:

private function isSymlinkAllowedForUser(): bool
{
  if (PHP_OS_FAMILY !== 'Windows') {
    return true;
  }
  exec('whoami /priv', $output);
  $isUserAllowedToCreateSymlinks = str_contains(join("\n", $output), 'SeCreateSymbolicLinkPrivilege');
  return $isUserAllowedToCreateSymlinks;
}

which is way slower and harder to read and possible not really accurate than the stfu @

// on windows creating "normal" symlinks is only allowed as admin or with SeCreateSymbolicLinkPrivilege
// in case we only want to symlink a dir, we can instead create directory junctions, which is allowed for non admins
// see https://github.com/git-for-windows/git/wiki/Symbolic-Links
exec(sprintf('mklink /j %s %s', escapeshellarg($temporaryTargetPathAndFilename), escapeshellarg($sourcePathAndFilename)));
Copy link
Member

Choose a reason for hiding this comment

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

Waiting on what edgecase this will break on, because escapeshellarg on Windows is haunted (it actually just replaces some characters with whitespace instead of escaping them - see https://www.php.net/manual/en/function.escapeshellarg.php). Also, escaping in Windows command shells is a hot mess in itself:

The % character is one exception to this rule, it is a valid NTFS filename character but also will be evaluated as a variable name even within a quoted string.
...
When the shell is running in EnableDelayedExpansion mode the ! character is used to denote a variable and so must be escaped (twice)
...
The \ escape can cause problems with quoted directory paths that contain a trailing backslash because the closing quote " at the end of the line will be escaped ".

https://ss64.com/nt/syntax-esc.html

So we might be better off just preg-replacing the important special chars with ^, putting double quotes around the two paths and making sure they end with a double-backslash if they do end with a backslash.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yay. But I guess Marc Henry has a point here - I myself has no windows availabale to test, but I'm fine be reading.

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

Successfully merging this pull request may close these issues.

None yet

5 participants