Merging pipe edits on the same file

[CPSibo]

Looking at LaravelFileFactory.calculateFiles(), only one pipe can ever touch a given file path and it can only ever generate whole files. If multiple pipes try touching the same path, you end up with duplicate keys in the array that gets shown in the Results tab and one of the generated files is essentially hidden.

This works more or less well enough for items like Models but prevents really working with the app-wide lower-level files like the service providers. If two pipes want to touch the same service provider by inserting different blocks of code in the boot() function, they can't, right now.

So I think the FileFactory/Pipes need to be able to insert select blocks of code at given points in a file. However, a file isn't guaranteed to exist, so they'd also need to be able to fall back to the current functionality of generating the entire file.

Imagine we have an AuthServiceProvider.php.stub like

<?php

namespace App\Providers;

use Illuminate\Support\Facades\Gate;
use Illuminate\Foundation\Support\Providers\AuthServiceProvider as ServiceProvider;

class AuthServiceProvider extends ServiceProvider
{
    public function boot()
    {
        Gate::guessPolicyNamesUsing(function ($modelClass) {
            return ___POLICIES_PATH___.class_basename($modelClass).'Policy';
        });

        $this->registerPolicies();
    }
}

This stub could be used by the Policy pipe that I sent in a merge request today since it'd allow the user to specify their desired Policies path under the Settings tab of the webapp. But some other pipe might also want to manually register a policy with this stub:

<?php

namespace App\Providers;

use Illuminate\Support\Facades\Gate;
use Illuminate\Foundation\Support\Providers\AuthServiceProvider as ServiceProvider;

class AuthServiceProvider extends ServiceProvider
{
    protected $policies = [
        Account::class => AccountPolicy::class,
    ];

    public function boot()
    {
        $this->registerPolicies();
    }
}

Our changes don't overlap but the current system prevents us from both registering policies.

---

There are three ways to solve this that I can think of:

1. Allow for partial stubs. Have LaravelFileFactory run calculateFiles() for all pipes and flatten the array, then for each pipe run the partial stubs against the calculated files, applying the changes specified.

The drawback of this method is when a file doesn't exist but only has a partial stub. Pipes with partial stubs would basically have to also provide the whole file stub as a fall-back, duplicating code.

---

2. Introduce some new markup options in the stubs. For instance, something like

<?php

namespace App\Providers;

use Illuminate\Support\Facades\Gate;
use Illuminate\Foundation\Support\Providers\AuthServiceProvider as ServiceProvider;

class AuthServiceProvider extends ServiceProvider
{
    ___[___
    public function boot()
    {
    ___]___
        Gate::guessPolicyNamesUsing(function ($modelClass) {
            return 'App\\Policies\\'.class_basename($modelClass).'Policy';
        });

        ___[___
        $this->registerPolicies();
    }
    ___]___
}

Where ___[___ marks the start of necessary scaffolding and ___]___ marks the end. The scaffolding is just the stuff immediately before and after the real changes of the pipe that need to exist for the change to work. But anything between or outside of those scaffolding bits can be changed by other pipes. If the file doesn't exist yet, the stub still has the entire file in it, so the extra markup can just be tossed out.

---

3. Use a diffing library to try to merge whole file stubs together. Basically, keep everything like it is now except for adding an extra step in LaravelFileFactory that finds duplicate path entries and tries to merge their content strings together.

This is probably easier than option 2 but would reasonably require adding an external dependency to some 3rd party JS diffing library. I doubt anyone wants to write a diffing facility just for this one spot in the code. It's also probably a bit more fragile than option 2 if two pipes touching the same file are written for different versions of Laravel, thus potentially having different file contents outside of the intended changes (eg. different use statements).

---

I'd be happy to code this functionality, but I'm not sure which direction you'd want the project to go in, if you even want this functionality at all.

[ajthinking]

Wow! thanks for this idea.
I think we want this, and you have some good suggestions. Not sure which is best. Which solution do you prefer?

Some questions/thoughts:

On 1)

Allow for partial stubs
Partial stubs exists somewhat, I think for relationships. Can you provide a minimal example for this? Not sure how you mean flatten the array

On 2)

What if you want to edit use statements or class definition - that is use templating outside the main class body / or have multiple sections?

On 3)

This might be the simplest solution together with a warning that there was a "merge conflict"?

At the moment all pipes use a ObjectModelCollection as input. But in some places that does not make sense, for example when a pipe is to be run just ONCE, or when it should be triggered by files from other pipes. Maybe a pipe should be able to accept/listen to different kinds of input. Example: the PolicyPipe could generate a policy file - but also send some notification object to other pipes that might be interested in responding. Maybe, this is similar to what you suggest in 1)?

[ajthinking]

@septIO any thoughts on this?

[CPSibo]

For option 1, by flatten the array, I mean your current call to .reduce() that flattens the 2D array (pipes x files) into a 1D array (files) for presentation on the Results tab.

An example might be something like
AuthServiceProvider.php.partial.stub

@InsertBelow('public function boot() {')
Gate::guessPolicyNamesUsing(function ($modelClass) {
     return 'App\\Policies\\'.class_basename($modelClass).'Policy';
});

I believe phpBB did something like this in the good ol' days for extensions that modified templates. It worked okay but they only ever modified files that were essentially certain to already exist, which isn't true for PipeDream. So, any pipe using partial stubs would also need the full stub as a fallback in case it just needs to create the entire file.

I guess PipeDream could have all the default framework files to supply as the fallback, rather than foisting that responsibility onto pipes but I'm guessing that won't scale well if you're wanting to expand PipeDream into other frameworks and languages.

---

For option 2, the "scaffolding" can be anything in the stub. The only problem would happen if two stubs try to modify the same line of code. So this example inserts a block in the boot() function and a use statement at the top of the file:

<?php

namespace App\Providers;

use Illuminate\Support\Facades\Gate;
___[___
use Illuminate\Foundation\Support\Providers\AuthServiceProvider as ServiceProvider;
___]___
use Some\Custom\Package;

___[___
class AuthServiceProvider extends ServiceProvider
___]___
{
    ___[___
    public function boot()
    {
    ___]___
        Gate::guessPolicyNamesUsing(function ($modelClass) {
            return 'App\\Policies\\'.class_basename($modelClass).'Policy';
        });

        ___[___
        $this->registerPolicies();
    }
    ___]___
}

It gets messy but I'm not sure if there's a more lucid syntax to use. Conceivably, this could also be done programatically, rather than declaratively. So something like:
PolicyPipe.js

let insertionBlocks = [
   {
       start: 'public function boot() {',
       end: '$this->registerPolicies(); }',
       content: "Gate::guessPolicyNamesUsing(function ($modelClass) {\
            return 'App\\\\Policies\\\\'.class_basename($modelClass).'Policy';\
        });"
   },
];

Then the pipe just has the normal .stub file either with or without the changes (your preference) and the pipe first checks for the file's existence. If it's in the array, just insert the blocks. If not, just write out the entire stub.

---

I agree that 3 is probably the simplest in terms of initial development. Though, it might require the user to actually resolve the merge conflict in the Review tab, which probably starts to raise questions you've entered in your trello about not overwriting user's manual edits in the Review tab when they change something in the design tab.

There are a few diff/patch libraries available for JS here on github but I'm not sure how accurate they actually are.

As for more advanced stuff like events, I had looked to see if PipeDream had that when I was writing the Policy pipe. For instance, if there was a service that could be queued to insert the policy auto-discovery code after all the pipes have been run, then it wouldn't matter how many pipes wanted to insert it. Or if I could declare that the AuthServiceProviderPipe was a dependency of the PolicyPipe and either do something to the dependency directly or let the FileFactory do something with it...

[ajthinking]

Thanks for laying this out in more detail, and please bare with me if I not grasp it fully yet to give you a clear answer

1

@InsertBelow('public function boot() {')
That syntax is pretty cool. So then multiple pipes can hook into that and insert the row one at the time.

Someone had the idea of moving the template to blade/mustash or some other engine - maybe thats similar to phpBB?

I guess PipeDream could have all the default framework files to supply as the fallback, rather than foisting that responsibility onto pipes but I'm guessing that won't scale well if you're wanting to expand PipeDream into other frameworks and languages.

I don't think that would be that a big of a problem? The pipes are already tightly connected with the templates by strings.

2

This is still a bit unclear to me, the section lack a name or identifiyer? But I do like the programatical approach. One downside might be that it is not obvious where things will appear when looking at the template? You would have to go to the review to actually see what the pipes will insert? That may not be such a big problem though

3

which probably starts to raise questions you've entered in your trello about not overwriting user's manual edits in the Review tab when they change something in the design tab.

I think that card might be refering to users making changes in their IDE and then coming back. Not sure.

---

As for more advanced stuff like events, I had looked to see if PipeDream had that when I was writing the Policy pipe. For instance, if there was a service that could be queued to insert the policy auto-discovery code after all the pipes have been run, then it wouldn't matter how many pipes wanted to insert it. Or if I could declare that the AuthServiceProviderPipe was a dependency of the PolicyPipe and either do something to the dependency directly or let the FileFactory do something with it...

Yes, the pipe certainly could have more power/intelligence than just simple templating..

septIO pointed out in chat you could also override the calculateFiles(). Maybe you already spotted that. Its inherited via the BaseFileFactory. By the way, if you want you are more than welcome to join the slack channel

[CPSibo]

1

This would be fairly user-friendly so long as there was a ready solution for when the file doesn't exist yet. If you're willing for the framework itself to house many of Laravel's default project files, then it'd probably work quite well. There could be other directives such as InsertAbove, InsertBetween, InsertIf, etc.

Moving to a templating engine could have benefits of its own but I don't think it'd solve this problem, since blades are top-down. At least with Blade, higher templates can include lower templates but lower templates can't forcibly insert themselves into higher templates. You'd have to code something else on top of blade to allow that kind of workflow.

2

Yeah, the syntax is basically like wrapping parts of the code in parenthesis so they don't really need names. So we'd write a simple lexer that could find matching pairs of the start and end tokens and then try to match the "scaffolding" bits to the same sub-strings in the file that's already in the array. When it finds the scaffolding, it inserts what's between the parenthesis, leaving anything else that's between the scaffolding already.

The programmatic version just skips the lexing step.

Well, users already can't see what the pipes are doing until they go to the Review tab. If you mean the pipe developers can't see what's going on then idk. They're already writing code for a webapp that abstracts something by several levels. I don't imagine it'd be a big issue for them.

---

I did see that but I doubt you'd want me customizing the global LaravelFileFactory just to make my one 3rd party pipe work a certain way. I'll try joining the slack channel when I get back from work today, thank.

[ajthinking]

I would not have a problem if a few extra files needs to be stored by the file factory so 1) seems good to me. 2) with programtic also good - not so sure about the lexing. 3) also good/easy, its at least an improvement.

Would be happy to accept a PR on either of those, your pick!