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

Same Uuid is re-used every time when creating a child page #6640

Open
ovenum opened this issue Aug 26, 2024 · 17 comments
Open

Same Uuid is re-used every time when creating a child page #6640

ovenum opened this issue Aug 26, 2024 · 17 comments
Assignees
Labels
type: bug 🐛 Is a bug; fixes a bug

Comments

@ovenum
Copy link
Contributor

ovenum commented Aug 26, 2024

Description

When a parent page contains a child page with slug new Kirby will re-use that child pages Uuid when a new child page is created.

Expected behavior

The Uuid to be unique : )

To reproduce

Using a fresh plainkit installation:

  1. Create a new Page from the panel named Some Page (name does not matter, it’s used just for reference)
  2. Manually create an unlisted child page with slug new and manually add a Uuid field
# /content/some-page/new/default.txt
Uuid: 7XTwwjop6ogKKCEW
  1. Using the Panel create another child page of Some Page
  2. Compare the Uuid of the child pages created via the panel to the Uuid of some-page/new.
    They are the same

Note
When the Uuid of the draft child page created via the panel is deleted manually, Kirby will correctly assign a new, truly unique Uuid to that page when it’s requested by the panel.

Your setup

Kirby Version
4.3.0

@distantnative distantnative added the type: bug 🐛 Is a bug; fixes a bug label Aug 26, 2024
@distantnative
Copy link
Member

I can reproduce the issue with the steps you've outlined. The new name seems to be crucial - renaming that sibling page to neww and a new UUID is correctly generated when adding a sibling.

@distantnative
Copy link
Member

@ovenum
Copy link
Contributor Author

ovenum commented Aug 27, 2024

Thanks for looking into this @distantnative.
Is the temporary page created by https://github.com/getkirby/kirby/blob/main/src/Panel/PageCreateDialog.php#L251 created in the filesystem and if so how is it removed, renamed?

I'm quite confident that a page with the slug new was not created by a user logged in to the panel. Is it possible that it’s an artifact of the method mentioned above that was not cleaned up properly?
The site in question had a few issues with oom-kill shutting down php-fpm, so this could have been caused by a very rare edge case. 🤷‍♂️

@distantnative
Copy link
Member

I'm quite confident that a page with the slug new was not created by a user logged in to the panel.

But isn't that your use case you outlined in the top post to reproduce this issue?

The temporary page https://github.com/getkirby/kirby/blob/main/src/Panel/PageCreateDialog.php#L251 should not be saved (and thus not create a new folder in the filesystem). But as currently our virtual pages are not properly restricted to in-memory, I assume if there already exists a page with the same slug, it will very willingly read the content file (and thus pick up the existing UUID).

@ovenum
Copy link
Contributor Author

ovenum commented Aug 27, 2024

But isn't that your use case you outlined in the top post to reproduce this issue?

Yes but only for the reproduction with the plain kit.

On the server where this issue was noticed, the child page with the new slug was in a folder that should only contain listed pages. That’s what got me to the reproducible example at the top.

I will check a migration script, used to re-sort child-pages, that was run in the past, if it could be responsible for the new page.

@ovenum
Copy link
Contributor Author

ovenum commented Aug 29, 2024

After deleting the page with the new slug twice it keeps reappearing in the content folder when content is edited. This is messing up UUID creation for pages created in the panel.

I noticed the same behaviour on another page. Still not sure what’s causing this but it was not the migration script or problems with oom-kill.

Not sure if the problems are related, so can create a another issue. Let me know.

@distantnative
Copy link
Member

Probably related and definitely important to catch the bug - at least it sounds like a bug in Kirby. We just need to figure out what brings out the bug.

Do you have any page hooks set up?

@ovenum
Copy link
Contributor Author

ovenum commented Aug 29, 2024

For the most recent site i’m using two hooks in my config file. Both sites share routes being registered from system.loadPlugins:after

    'hooks' => [
        'system.loadPlugins:after' => function () {
            kirby()->extend([
                'routes' => include __DIR__ . '/config.routes.php'
            ]);

            /**
             * Register PHP Blueprints from Kirbys Blueprints directory
             */
            kirby()->extend([
                'blueprints' => Blueprints::loadPhpBlueprints()
            ]);
        }
    ],  
#  config.routes.php

return (function (): array {
    $merchantsPage = kirby()->page('markt');
    $merchantCategoriesPage = $merchantsPage->children()->findBy('intendedTemplate', 'merchant-categories');

    return [
        [
            'pattern' => "(:any)",
            'language' => '*',
            'page' => $merchantCategoriesPage->id(),
            'action' => function (string $language, Page $page, $category) use ($merchantsPage) {
                $categoryPage = $page->children()->findBy('slug', $category);

		// Return 404 when category page is not found
                if (!$categoryPage) {
                    return false;
                }

		return $merchantsPage->render([
		  'category' => $categoryPage
		]);
            }
        ],
    ];
})(); 

Plus hooks added by tobimoris DreamForm, thumbhash and SEO plugins.

I can rule out the DreamForm plugin because it was added to the site after the new folder was created.
The thumbhash plugin adds two file hooks: file.update:before and file.replace:before.
Seo Plugins adds these:

return [
	'page.update:after' => function (Page $newPage, Page $oldPage) {
		foreach ($newPage->kirby()->option('tobimori.seo.robots.types') as $robots) {
			$upper = Str::ucfirst($robots);
			if ($newPage->content()->get("robots{$upper}")->value() === "") {
				$newPage = $newPage->update([
					"robots{$upper}" => 'default'
				]);
			}
		}
	},
	'page.render:before' => function (string $contentType, array $data, Page $page) {
		if (option('tobimori.seo.generateSchema')) {
			$page->schema('WebSite')
				->url($page->metadata()->canonicalUrl())
				->copyrightYear(date('Y'))
				->description($page->metadata()->metaDescription())
				->name($page->metadata()->metaTitle())
				->headline($page->metadata()->title());
		}
	},
];

@ovenum
Copy link
Contributor Author

ovenum commented Aug 30, 2024

Some more information regarding the issue. Two post above i was talking about two pages where the issue re-appeared, which is unclear, i was referring to two different Kirby installations.

  • They both run on Hetzner VPS in the same Datacenter.
  • Multiple users are adding new page drafts to the same parent page at the same time of day. The accounts are not shared, every user has it’s own login.

@ovenum
Copy link
Contributor Author

ovenum commented Aug 30, 2024

More observations from today : )
The issue is now reproducible every time a page draft is created. On the live installation and on my local machine after syncing the content folder.

  • The page draft is correctly created with a unique UUID.
  • A page with the slug new is created as an unlisted page with a different UUID then the page draft that has been created.
  • When another page is created via the panel it will get the UUID of the unlisted page with slug new. As described in the issue description

Deleting the uuid cache folder and populating it does not make a difference. I’ve checked via script that there are no duplicate UUIDs before testing with the steps above.

Connecting x-debug with a breakpoint at https://github.com/getkirby/kirby/blob/main/src/Panel/PageCreateDialog.php#L251
i noticed that the PageCreateDialog::model is called multiple times in two requests.

The first request is happening when the Page Create Dialog is opened, the one where Kirby asks for the page title.
After confirming by clicking on Create as draft a second request is issued calling PageCreateDialog::model multiple times, on the last call the unlisted new page is being created.

@distantnative
Copy link
Member

@ovenum Thanks that you keep posting your observations.

I haven't had the chance to reproduce this myself but all this contextual information will surely help.

@ovenum
Copy link
Contributor Author

ovenum commented Aug 30, 2024

No problem. This is not meant to pressure you. It’s more thinking aloud or a mini journal documenting the observations.
Let me know when you need more specific information, stack trace, etc…

@afbora
Copy link
Member

afbora commented Sep 9, 2024

@ovenum Can you share a copy of your app here or via [email protected] so we can test where the temporary page actually created in content folder?

@ovenum
Copy link
Contributor Author

ovenum commented Sep 11, 2024

@afbora that could be difficult, the complete installation is around 22GB in size (lots of images) and includes forms with privacy relevant data which i would have to remove prior.

I have time today to take a closer look at this issue again and can share stack traces leading up to the creation of the new page and can try to create a smaller example installation that reproduces the issue which i can share.

@ovenum
Copy link
Contributor Author

ovenum commented Sep 11, 2024

@afbora could create a reproducible example repo based on the plainkit.

https://github.com/ovenum/kirby-uuid-issue-reproducible

  • Log into the panel
  • navigate to test-parent page
  • click Add in the pages section and create a page draft
  • check /content/test-parent folder for the new page and the _drafts dir for the actual created draft.
  • They will not share the same UUID yet
  • Create another draft page from test-parent
  • check the created draft page uuid: it will have the same uuid the new page

Expected Behavior update

  • No unlisted new page should created when creating a page draft.

Observations
The Test page (child pages of test-parent) includes a custom field with a value prop that will call relatedPagesPanel method on the fields model. relatedPagesPanel calls Page::uuid, this call is at the root of the issue. When you remove the call, no new page will be created

The custom field value prop:
https://github.com/ovenum/kirby-uuid-issue-reproducible/blob/778507a4e97a03b605e6c320222337066297767d/site/plugins/test-field/index.php#L13-L19

The page model function that’s called by the custom field
remove the call to $this->uuid()->toString() here and things will work as expected
https://github.com/ovenum/kirby-uuid-issue-reproducible/blob/778507a4e97a03b605e6c320222337066297767d/site/models/test.php#L8C1-L21

Sorry code embeds don’t seem to work for me(?)

@afbora
Copy link
Member

afbora commented Sep 11, 2024

Thank you @ovenum

I had discussed this issue with @distantnative before. I said that the issue was related to calling ->uuid(). My suspicions were correct.

Regarding the solution, I'm not sure about a permanent solution but as a temporary solution I would suggest the following usage instead of $this->uuid()->toString().

// workaround solution for the uuid issue
$this->content()->get('uuid')->value();

@ovenum
Copy link
Contributor Author

ovenum commented Sep 14, 2024

Can confirm that reading the uuid from the content file instead of calling Page::uuid does stop Kirby from creating a second page with new slug.

For people reading this keep in mind that when reading the UUID from the content file, it will not have the UUID scheme (page://, file://) prepended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

No branches or pull requests

3 participants