Can a return value be avoided as it is a "set" method? what gets modified here?

The event can modify the list of sites and this function is called in SitesManager API. So if we don't return here, the API output isn't affected by any changes in the event.

So if a plugin changes the name of a site, it will be reflected in the Site class cache, but not in SitesManager API output (and thus not in, eg, the site selector).

Would it be possible to call Sites::getSites instead of returning a value there? It is really not expected that a setter method changes the value and returns a different value. Maybe getSites would under circumstances return too many sites?

I suppose it would work for getting all sites, but I don't think it would work when working w/ a subset of all sites (eg, getting one site or the pattern match sites method).

The alternative is to pass sites in by reference, but that could be a BC break. Personally I don't see an issue w/ it returning since it's not really setting a property but adding sites to a cache. Perhaps it would be better to rename this method to something like Site::addToSiteCache() and keep Site::setSite() as deprecated proxy to new method (for BC).

I think it would be still quite unexpected. Could we maybe expose the method triggerSetSitesEvent somehow but possibly with a better name? Site::enrichSites or something? The set method does currently two things, enriching plus setting. We only want the "enriching" part so could just expose that method? Wouldn't be too much better as you would maybe already expect to get a site "enriched". It also means the "enriching part" would happen twice and possibly lead to problems? So it might not be an option either.

So OK to use the regular set method as already done. Let's just add a comment to the method to make clear what is happening and I wonder if we should mark the method as deprecated plus internal or so but not too important maybe as long as there is a good comment.