Update social-images route to relative path

[SanderGo]

Hi Rob,

Today I encountered a problem with the /social-images/{id} route returning a 404 error page. I managed to find the issue and figure out a fix.

I've configured my multisite url's per 'best practise' as stated here: https://statamic.dev/multi-site#url
So, using the fully qualified, absolute URL.

The route generated for the /social-images now includes that full url in the route:list. That's what causes the 404.

For me, it was fixed by removing the $site->url() in the web.php (line 28).

Route::statamic("{$site->url()}/social-images/{id}", 'statamic-peak-seo::social_images', [ 'layout' => null, ]);

to

Route::statamic("/social-images/{id}", 'statamic-peak-seo::social_images', [ 'layout' => null, ]);

Not sure why the site url was included there? I could just revert my multisite url to '/', but then we would be missing out on best practices ;-)

Curious to hear your point of view. If you agree I could PR the fix as well.

Greets, Sander

[robdekort]

That advice is new. Also Statamic doesn't ship with that right? So how could be a best practice I wonder 🤔.

Does the Route without $site->url work with how Statamic ships by default (not with the absolute URL). If so I guess we'd better remove it.

[SanderGo]

That's correct, it ships with a default '/' for the default site url. They don't really mention in what case, using the absolute url, would be considered 'best practise'. I was hoping you would have the answer to that :)

I will do a clean Statamic/Peak install and see if I can figure out a bit more on this. I will also test to see if my update would break anything on different configs.

[robdekort]

Cool that’s wonderful. Keep me posted :)

[SanderGo]

Alrighty, did some testing and poking. Long story below with my findings, but in short: the absolute url's are just breaking the Social Images Generation functionality. Removing the {$site->url} will break the social-images url for any sites with a full domain..

====

A clean install of Statamic/Peak, with Social Images Generation enabled, activates this route:

GET|HEAD  social-images/{id}  ............................. Statamic\Http › FrontendController@route

The default site config in config/statamic/sites .default is:

'default' => [
            'name' => config('app.name'),
            'locale' => 'en_US',
            'url' => '/',
        ],

When converting to multisite through php please multisite and adding a dutch site, the default url remains '/' and the dutch site's url is set to /dutch/. So that's the default behaviour.Also, the social-images routes doesn't change.
When I update the site's urls as per 'best practices', there are two routes for 'social-images', both full, absolute urls. 

GET|HEAD  http://my-site.test/dutch/social-images/{id} ....... Statamic\Http › FrontendController@route
GET|HEAD  http://my-site.test/social-images/{id} ..................Statamic\Http › FrontendController@route

When I return the '/' url for the default site, but add a full, absolute url for the second site (like 'my-dutch-site.nl' in case you would like to use a different domain for the second site, as per documentation), this would result in these two routes:

GET|HEAD  https://my-dutch-site.nl/social-images/{id} ......Statamic\Http › FrontendController@route
GET|HEAD  social-images/{id} .............................................Statamic\Http › FrontendController@route

When I remove the {$site->url} from the route file, there is only 1 route listed:

GET|HEAD        social-images/{id} ................................... Statamic\Http › FrontendController@route

But I guess that would break the generating of Social Images for the 'dutch' / second site, right? 

So, I'm guessing this isn't a good solution after all. Any idea on how to support the full, absolute url's? I'm running a multisite with different domains for language. Any advice on a better/different setup is welcome as well. 

[robdekort]

I see. I'm not sure tbh. Maybe @marcorieser has some thoughts on this if he has time.

[marcorieser]

Hopefully I have some time to have a look at it tomorrow. First thoughts:

• Why is it a Statamic route instead of just a Laravel one?
• Routes do not need the domain/host prefixed in the definition, so starting with '/' should be fine.
• I believe the custom routes in the other addons do start with just the slash too.

[SanderGo]

Thanks Marco, it got me thinking and I did some more testing.

• I believe we need the Statamic route here to pass the layout: null parameter.

I've set up a clean multisite install with two sites, using the full, absolute urls. Then I've tested generating the Social Images again (without the {$site->url} present) and it works just fine for both sites/languages!

Looks like we don't need the {$site->url} because it's looping through the sites and then visiting the social-images url, which then presents the corresponding site's output.

I guess we're back to my first proposal to just remove the {$site->url}, as it seems to work fine in all cases (single site, multi-site, with relative domains and with full absolute urls).

[robdekort]

Nice finds Sander. I'll try to dive in soon and will remove it if my test results are the same :-).

[SanderGo]

Thanks Rob, I've been working on for a bit more, found some edge cases where it would break. Think I might have another solution, will post soon when I did some more testing!

[robdekort]

Ah cool, I'll await that then.

[marcorieser]

Will chime in after you tinkered around.

[SanderGo]

Thanks guys, I figured I'd better let you know not to waste any time :)

I did a few hours of digging and testing just to make sure it would work in all combinations of configurations.
It looks like just removing the {$site->url()} will break multisites with a relative url..

The updated solution I found seems to be working good, solving the full absolute path problem, while still supporting the relative urls.

use Statamic\Facades\URL;

Route::statamic(URL::makeRelative($site->url())."/social-images/{id}", 'statamic-peak-seo::social_images', [
            'layout' => null,
        ]);

It generates the Social Images for both sites, with the correct content. Either working with full absolute urls or relative urls.

The only issue I'm running into is that it breaks when using a full absolute 'sub-folder' domain ('url' => env('APP_URL').'fr/') for your site url, as stated in the 'best practices'. The full absolute url ('url' => 'my-french-site.fr') works when it's a full but different domain. The relative 'sub-folder' url ('url' => 'fr/') works fine as well.

To be clear, with 'breaking' I mean the '/social-images/{id}' page is loading, but the site is incorrectly set to 'default' and for that reason the content is not found.

If you guys have any ideas on a fix for this, that would be great. Or, maybe those 'best practises' in the docs might just be not so 'best' after all?

[marcorieser]

@SanderGo Any chance you are in the Peak Discord?

[SanderGo]

I'm not, but sounds like a good idea.. on my way!

[marcorieser]

Now that my binary problem is solved (#19) hopefully I can have a look at it tomorrow 😁

[SanderGo]

FYI: I've updated the package on my Forge production server and then applied the fix (URL::makeRelative). Runs great for all languages

[marcorieser]

So is it fixed for now? If not, would be nice to have a list of the things you successfully tested (how you setup the multisite domains) and what the remaining issues are.

[SanderGo]

Sorry, maybe I wasn’t clear.. I hardcoded my fix on the server, the one I proposed lastly. Just wanted to let you know this was successful. So it’s not fixed, not until I can PR this and it gets merged :)

[marcorieser]

Another approach than registering the route for every configured site:
We currently pass the route the ID of the entry, which is unique. Why dont we just set up the route for the first site configured and handle everything via that. No idea if that causes side effects since we could be in the wrong site scope but basically: who cares if the route is available for each site? Just some thoughts than fixing all those details with possible urls. Anyways: why dont you just PR the changes (as long as they work in single site setup too) and we fix future issues if they occur?

[SanderGo]

I figured it was to load the correct site scope indeed; the content with given id is not available in the other site scope(s).
But I will make sure to test it, just to be sure.

I will create the PR and include all my test cases, as soon as I will find a moment.

[marcorieser]

Is implemented by #23

[robdekort]

Thanks for all your work guys. Love it. Have a great weekend.