Fix issue #14 by modifying register callback

[eddiewebb]

I dont know if it is a pass by reference/copy issue, but modifying the array within a resolving callback was ignored and so any custom resource classes registered there were ignored. (see issue FriendsOfFlarum#14)

but per illuminate docs we can wrap and augment existing instances using extend.

I left debug logging in place to see differec.

Before PR - extension resource was not included by SitemapGenerator

[2020-07-21 23:17:17] production.DEBUG: [Webbinaro\AdvCalendar] Resource Initialized  
[2020-07-21 23:17:17] production.DEBUG: [RegisterResource::extend] added Webbinaro\AdvCalendar\Integrations\EventResource  
[2020-07-21 23:17:17] production.DEBUG: [RegisterResource::extend] resolved 4 resources.  
[2020-07-21 23:17:17] production.DEBUG: [RegisterResource::extend] afterResolving 3 resources.  
[2020-07-21 23:17:17] production.DEBUG: [SitemapGenerator::getUrlSet] resolved 3 resources. 

^ note that the afterResolving callback and any calls to make (including generator's) do not include the modification to resources array. (i think php copy-by-value-on-write quirk, i dont know, it didnt work)

After PR - modifications made by RegisterResource are picked up by Sitemap Generator 🎉

[2020-07-22 00:00:55] production.DEBUG: [Webbinaro\AdvCalendar] Resource Initialized  
[2020-07-22 00:00:55] production.DEBUG: [RegisterResource::extend] added Webbinaro\AdvCalendar\Integrations\EventResource  
[2020-07-22 00:00:55] production.DEBUG: [RegisterResource::extend] resolved 4 resources.  
[2020-07-22 00:00:55] production.DEBUG: [RegisterResource::extend] afterResolving 4 resources.  
[2020-07-22 00:00:55] production.DEBUG: [SitemapGenerator::getUrlSet] resolved 4 resources.  
[2020-07-22 00:00:55] production.DEBUG: [Webbinaro\AdvCalendar] Resource Queried  

[clarkwinkelmann]

Thanks for looking into this. It seems like the issue is indeed due to a value vs reference mistake.

I believe just replacing function (array $resources) with function (array &$resources) would also fix the issue? If so, I'd prefer that option so that we can keep afterResolving().

[eddiewebb]

@clarkwinkelmann I did try changing definition to &$array but no change, I don't know why, but it didnt stick.

Using ->extend to change it, calls to afterResolving() worked in my test. Locally I have

        $container->afterResolving('fof.sitemap.resources', function ( &$resources, $app) use ($container) {
            // conffirms our modification is shared
            app("log")->debug("[RegisterResource::extend] afterResolving "  . count($resources) . " resources.");
        });

and result is consistent, so if the plugin is using afterresolve(), it should be fine.

[2020-07-22 23:11:27] production.DEBUG: [Webbinaro\AdvCalendar] Resource Initialized  
[2020-07-22 23:11:27] production.DEBUG: [RegisterResource::extend] added Webbinaro\AdvCalendar\Integrations\EventResource  
[2020-07-22 23:11:27] production.DEBUG: [RegisterResource::extend] resolved 4 resources.  
**[2020-07-22 23:11:27] production.DEBUG: [RegisterResource::extend] afterResolving 4 resources.**  
[2020-07-22 23:11:27] production.DEBUG: [SitemapGenerator::getUrlSet] resolved 4 resources.  
[2020-07-22 23:11:27] production.DEBUG: [Webbinaro\AdvCalendar] Resource Queried

[clarkwinkelmann]

I see Flarum used extend() for arrays as well, so this might indeed be the only solution https://github.com/flarum/core/blob/master/src/Extend/Middleware.php

[eddiewebb]

I removed the extra logging lines only function change now if you want to merge.

[clarkwinkelmann]

Thanks! I have published version 0.5.2 with the fix.