in2code-de / in2publish_core

in2publish Community Version
https://www.in2code.de/produkte/content-publisher/
GNU General Public License v3.0
40 stars 23 forks source link

sys_redirect with same source_host may prevent synching across systems #118

Open dreistromlandMf opened 2 months ago

dreistromlandMf commented 2 months ago

Assume you have a sys_redirect where every field, including the source_host, is identical on booth systems. This may be because you transfered one database backup to booth systems.

This will be cought by SysRedirectDatabaseRecord::calculateChangedProps - it doesn’t make sense that booth system have the same source_host, so in2publish assumes that booth have been reset onto the local system and foreign needs to be updated. The props are identical at this point.

In this case RedirectSourceHostReplacement::replaceLocalWithForeignSourceHost will attempt to figure out the URL based on tx_in2publishcore_foreign_site_id, tx_in2publishcore_page_uid, or target. Assume that the first two don’t yield results, and that the target also does not exist on the foreign system. This would mean the function returns at line 118. The properties would not be changed in this function either, there are still no changes to be published.

Nontheless, the publishing resumes. There is no further check if the props are identical in all systems. DatabaseRecordPublisher::publish produces an empty array of changes and proceeds to issue an update. Doctrine will do as told and produce an UPDATE `sys_redirect` SET WHERE `uid` = ? with no values after SET, which the database does not like. This results in a failed database query, which results in an abnormal end of the program flow at this point with an error message indicating an SQL error.

Even tho this is produced by an inconsistent database due to bad deployment practices, it’d probably be a good idea to at least produce a more meaningful error message. Or check if the props are empty at some point after the source_host should have been updated and not attempt the change.

tinzog commented 2 months ago

Thanks for opening this issue. We will investigate this as soon as possible .

sbusemann commented 2 months ago

https://projekte.in2code.de/issues/66386

dreistromlandMf commented 1 month ago

It looks like this can cause further issues: If a redirect has changed but the site is not resolvable, the source_host on foreign will change to the local url. This not only breaks the redirect, but also causes the original reported issues to happen sooner or later.

I have been able to produce such behaviour by changing the path segment of a page containing disabled pages. The newly created redirects for the disabled subpage don't have a site and apparently cannot be resolved.

dreistromlandMf commented 2 weeks ago

We have solved that internally via a CollectReasonsWhyTheRecordIsNotPublishable event listener. In there, we compare the source_host of the SysRedirectDatabaseRecord against the domains from Sites.

I don’t think the solution we currently deploy can be used 1:1. We ensure that the domain is different than the Sites on local. If it is the same as any local site, we abort publishing. I am not sure this would work on instances where local is on the same domain, but a different path. I would thus recommend ensuring that the domain is the same as a foreign site, instead.

The solution has been deployed on production for some time now. We did not have any issues since.

CollectReasonsWhyTheRecordIsNotPublishable event listener ```php final class SkipRedirectsWithIdenticalSourceHosts { public function __invoke(CollectReasonsWhyTheRecordIsNotPublishable $event): void { $record = $event->getRecord(); if (!($record instanceof SysRedirectDatabaseRecord)) { return; } $localSourceHost = &$record->getLocalProps()['source_host']; if (!isset($localSourceHost)) { // I'm not sure if this can happen return; } if (!$this->isLocalDomain($localSourceHost)) { return; } $event->addReason( new Reason( 'LLL:EXT:site_package/Resources/Private/Language/locallang.xlf:in2publish.reason.source_host', ), ); } private function isLocalDomain(string $localSourceHost): bool { static $sites = null; if ($sites === null) { // ToDo You probably want to compare against the sites on foreign instead // and invert the condition. $siteFinder = GeneralUtility::makeInstance(SiteFinder::class); $sites = array_map( fn(Site $site) => $site->getBase()->getHost(), $siteFinder->getAllSites(), ); } return in_array($localSourceHost, $sites); } } ```