Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys_redirect with same source_host may prevent synching across systems #118

Open
dreistromlandMf opened this issue Aug 19, 2024 · 4 comments
Assignees
Labels
Milestone

Comments

@dreistromlandMf
Copy link
Contributor

dreistromlandMf commented Aug 19, 2024

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
Copy link
Contributor

tinzog commented Sep 4, 2024

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

@sbusemann
Copy link
Contributor

@dreistromlandMf
Copy link
Contributor Author

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.

@sbusemann sbusemann added this to the in planning milestone Oct 28, 2024
@dreistromlandMf
Copy link
Contributor Author

dreistromlandMf commented Nov 1, 2024

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
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);
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants