What about asking until a valid path is given ?
In case the user made a typo and the path is invalid, we ask him again with the value he just gave us as default.

function! s:askForAPath(defaultValue) abort
  let l:manualPath = input('Enter file path: ', a:defaultValue, 'file')
  redraw

  if !empty(glob(l:manualPath.'/'))
    return l:manualPath
  endif

  " We might be able to echo here in order to let the user know that the path is invalid
  " If it does not work or is not clean enought, add a flag as parameter or the promp directly
  " And add a short message like "Invalid path\n" to preprend to the prompt
  return s:askPath(l:manualPath)
endfunction

It's a matter of taste and habit but personally I prefer to use recursion in those cases.
I find it more readable and easier to understand than a loop with break points in it.
Resolve if you don't like it, it's purely syntactic :)
You could even regroup the two conditions and gain 3 lines but I prefer readability.

function! s:searchDirectoryUpwardForRootMarkers(directory, workspaceRootMarkers, filesystemRootMarkers) abort
  if 0 <= index(a:filesystemRootMarkers, a:directory)
    return a:directory
  endif

  if s:directoryMatchesToPatterns(a:directory, a:workspaceRootMarkers)
    return a:directory
  endif

  return s:searchDirectoryUpwardForRootMarkers(
    \ fnamemodify(l:directory, ':h'),
    \ a:workspaceRootMarkers
    \ a:filesystemRootMarkers
  \ )
endfunction

To be equivalent it (not counting minor mistakes) should return v:null when encounter one of a:filesystemRootMarkers.

I guess the idea was better safe than sorry with the copy but we can access it directly through the dictionary.
So could we imagine let the user the care to copy the projects before playing with them in order to avoid making copies every time the method is used (even if it's only to read) ?
And while at it, duplicate with dict.projects, choose one of the two if we decide to optimize and remove the copy.

We can copy projects dictionary within PhpactorListProjects implementation instead.

I didn't look much at the command to be honest and now that I do I don't get it, the map() seems of no use.
And returning only the list of paths doesn't seem that useful.
I would rather remove it for now and wait for a need to emerge before implementing it: YAGNI

And even we keep it there is no need to copy it.
The all point of making a copy is to make sure that we don't modify the projects we got internally, but this command only do "read actions".

Maybe my need for ensure what projects already exist is more specific for development than normal usage ;)

We could also imagine expanding this to "switch" projects (in the future, better to keep this PR small :) )

Given a project does not contain any data it's on no consequences if I try to register the same project twice, isn't it ?
If I'm not mistaken then I propose to fail silently to prevent user experience:

Here you don't have to check the all structure but at least that the function exist and is a function.
But in order to avoid repeating the same checks and to keep the knowledge at the same place I encourage your to create a function phpactor#project#project#isValid(project) that will do all the necessary tests.

So the goal of the forbidden directories is to say that any of their sub-directories can never be linked to a project ?
I would have expected something like:

I took the liberty to use some more meaningful names in order to represent a real PHP scenario.
So if I'm in a project and I open a file from one of my vendors, which also contains the composer.json marker file.
Then I would like to be able to consider all my vendors as being part of my project instead of being their own projects.

I don't know if I make sense...

The purpose of excluding some directory from be a project root is an attempt to avoid indexing large directory that is supposed to be not a project root. / and $HOME are usual examples. If searching upward stops at one of excluded directories then there is no reasonable value to suggest and another resolver should be used. In non-interactive variant (g:phpactorAllowInteractiveProjectResolution == v:false) the next resolver will be used (initialCwd is always present in g:phpactorNoninteractiveProjectResolvers independently of user settings).

It is not intended to disable any marker from be stop in moving upward so I agree with your scenario except the last assertion.

However, if a fallback directory (initialCwd) is one of excluded (e.g. we start from ~/) and will be used, then we cannot avoid it.

The case added by you is a good example to show that searching upward to any root marker is not always a good resolver. Assuming additionally that we start (and still have CWD) in home directory or one of excluded, we also have a good example to show that initialCwd is not god resolver for this case. Thus I added manual method and an option to enable interactive choice from all methods (let g:phpactorAllowInteractiveProjectResolution = v:true).

OK so it's not meant to say "Ignore the vendor as project root and search upwards", it meant to say "Stop right now there is no interest to look anymore".

At first I thought it was an option to avoid taking the vendor directory into account, hence my misunderstanding.

If it's global it should be in plugin/phpactor.vim

Right but I have doubt in which section it should land (configs or none). It does not keep any simple configuration.

I think we should improve that, the goal is that it should be understandable by someone not knowing how everything works.
It's an help to show the user how the plugin will behave and hint on how to configure it if needed.

So first, I would add an information stating which kind of root detection is used.
Then you could add one function per type of detection, each one will have the responsibility to add relevant information.

For instance, we should not bother someone with the F being part of shortmessage or not if it does not impact his current configuration.

You should also use the {advice} parameter for health#report_warn and health#report_error in order to explain why the F option is problematic and only in the case of the manual detection.

This will solve the issue #985
But I think we can do even better, right now it only makes possible to use phpactor root detection system.

It should not be phpactor responsibility to resolve the directory in the first place, we should only ensure that we provide the root directory when sending a request.

I mentioned it because on this line I think we should consider the all root detection mechanism as external.
It would reduce the complexity by getting back to real need: provide a root directory.
Initially phpactor was using g:phpactorInitialCwd, to prevent any BC break we need to keep this variable with it's value being the directory from which vim was started.
From this point we want to be able to provide a directory depending on the buffer, meaning we will need to "ask" each buffer what is its root directory.
We can translate this need by

We want to be able to apply different strategies at runtime

So instead of coupling this piece of code to any system, let's couple it to an abstract strategy that could be defined by anyone.

With

if !exists('g:PhpactorRootDirectoryStrategy')
  let g:PhpactorRootDirectoryStrategy = {-> g:phpactorInitialCwd}
endif

function! phpactor#getRootDirectory() abort
  let l:Strategy = get(b:, 'PhpactorRootDirectoryStrategy', g:PhpactorRootDirectoryStrategy)

  if type(function('type')) != type(l:Strategy)
    let l:Strategy = {-> g:phpactorInitialCwd}
  endif

  return l:Strategy()
endfunction

At this point the user is free to define both global and buffer local strategies.
We don't have any BC break since the default behavior is the same as before the first attempt to resolve the root directory.
Example of configuration:

To use with your system

" vimrc
let g:PhpactorRootDirectoryStrategy = {-> get(get(b:, 'phpactorProject', {}), 'primaryRootPath', g:phpactorInitialCwd)}

To use vim pwd (for users with vim-rooter for instance)

" vimrc
let g:PhpactorRootDirectoryStrategy = function('getcwd')

The goal of this proposal is not to diminish your work at all, it's simply to provide a way of integrating in a extensible way.
So thanks for all what you have done anyone can use phpactor out of box with multiple projects in one vim instances.
Users having already a setup handling this notion can plug into phpactor to reuse what they have.
Users with no need for it will keep things as they were.

I kept such idea (without any implementation) as a backup in case of non-acceptance the whole work, but it should be the default approach - not backup. Your proposition sounds reasonable: gives an abstraction that contains a minimal implementation and can be replaced by any implementation maintained independently in some separate project.

The whole implementation of this approach is yours so please make PR that closes this after merge.

@dantleech @elythyr thanks for a lot of critical word.

Thanks so much for all your work so far :) Even if this PR isn't merged as-is there is lots of valuable stuff here which we can take forward (especially with the Vader tests).

I've found it quite difficult to follow everything going on here, but I think we could do something like:

• Allowing project-root strategy to be selected (custom or phpactor CWD)
• Strategy for changing CWD if the user opens up a file != the filesystem branch of the current project root (with confirmation).

Either as part of this PR or new PR(s) - would that cover everything?

Let this PR die in the current form and (before) implement @elythyr's proposition in a separate one.

@elythyr would you like to make such PR? I can do it just as well.

You can do it if you want.
The ideal would be to do it quickly enough so it will be in the next release.

Let me know if you won't have the time.

It can be unpredictable so we can assume I have not.