Skip to content

Deprecate optional parent prop in Cms\File#3372

Merged
lukasbestle merged 5 commits into
developfrom
fix/file-create-parent-optional
Jun 7, 2021
Merged

Deprecate optional parent prop in Cms\File#3372
lukasbestle merged 5 commits into
developfrom
fix/file-create-parent-optional

Conversation

@distantnative
Copy link
Copy Markdown
Member

@distantnative distantnative commented May 26, 2021 •
edited
Loading

Status: ready for review 👀

Release notes

Deprecated

  • Creating a Kirby\Cms\File without a parent property has been deprecated and throws a warning. Starting in 3.7.0 the property will be required and cause a breaking error if not passed.

Initial post

This getkirby/getkirby.com#1348 issue for the website pointed out that the parent prop should be listed as required, since the File::create() method will throw an exception if it is not passed.

However, looking at the code I could not figure out why it should be required. We only seem to pass it to File::factory() which creates a new File object. There the property setter File::setParent() states the property as optional.

@lukasbestle @bastianallgeier
Do you remember maybe why this Exception was put there to require the parent prop?

@distantnative distantnative added needs: discussion 🗣 Requires further discussion to proceed type: bug 🐛 labels May 26, 2021
@distantnative distantnative self-assigned this May 26, 2021
@distantnative distantnative force-pushed the fix/file-create-parent-optional branch from 6b04afa to de6ea7e Compare May 26, 2021 18:30
@distantnative distantnative mentioned this pull request May 26, 2021
Closed
@lukasbestle
Copy link
Copy Markdown
Member

lukasbestle commented May 26, 2021

Yes, because several methods like File::root(), apiUrl() and mediaRoot() rely on the parent object.

@distantnative
Copy link
Copy Markdown
Member Author

distantnative commented May 26, 2021

But is then the mistake that File::setParent treats it as optional?

@lukasbestle
Copy link
Copy Markdown
Member

lukasbestle commented May 26, 2021

Yes, I think so. Either setParent should require the property or all methods that rely on it need to have a fallback if it isn't defined. I think the first option would be the easiest, but I'm not sure about possible breaking changes.

@bastianallgeier
Copy link
Copy Markdown
Member

bastianallgeier commented May 27, 2021

I also think that it's a mistake in setParent. We don't have any isolated files that don't have a parent. The only ones are assets and they are handled by a different class.

@distantnative distantnative force-pushed the fix/file-create-parent-optional branch from de6ea7e to 30aa3d7 Compare May 27, 2021 17:13
@distantnative distantnative changed the title File::create(): make parent prop optional File: parent prop needs to be required May 27, 2021
@distantnative
Copy link
Copy Markdown
Member Author

distantnative commented May 27, 2021

@lukasbestle @bastianallgeier I have tried that (see new commit), but at least in a larger number of our tests, parent is also treated as optional. So not sure if this won't be a big breaking change for others as well.

github-actions[bot]
github-actions Bot reviewed May 27, 2021
View reviewed changes
Comment thread tests/Cms/Files/FileActionsTest.php Outdated
@lukasbestle
Copy link
Copy Markdown
Member

lukasbestle commented May 27, 2021

Are there use-cases where it would make sense to manually instantiate a File object from scratch besides our tests? As long as the files are only accessed from the site tree, the parent is set automatically anyway.

@distantnative
Copy link
Copy Markdown
Member Author

distantnative commented May 27, 2021

Virtual files?

@lukasbestle
Copy link
Copy Markdown
Member

lukasbestle commented May 27, 2021

Yeah, you are right. These do have parents (their virtual or real page), but it could well be that users are manually instantiating the files without passing the parent property explicitly.

@distantnative
Copy link
Copy Markdown
Member Author

distantnative commented May 31, 2021

It's really tricky cause I feel either way it would be a breaking change. But the current state - File::create() treats it as required, File::setParent() not - seems like a bug as well.

@lukasbestle
Copy link
Copy Markdown
Member

lukasbestle commented Jun 3, 2021

Yep, currently a file object without parent is of limited use as many methods will not work if the parent is not defined.

Maybe we should deprecate a missing parent property in 3.6.0 (with deprecation warning) and then enforce it in 3.7.0.

@distantnative distantnative force-pushed the fix/file-create-parent-optional branch from 30aa3d7 to f267086 Compare June 5, 2021 18:17
@distantnative distantnative changed the title File: parent prop needs to be required Deprecate optional parent prop in Cms\File Jun 5, 2021
@distantnative
Copy link
Copy Markdown
Member Author

distantnative commented Jun 5, 2021

@lukasbestle I have adapted the PR as suggested

@distantnative distantnative added this to the 3.6.0 milestone Jun 5, 2021
@distantnative distantnative added type: refactoring ♻️ and removed needs: discussion 🗣 Requires further discussion to proceed type: bug 🐛 labels Jun 5, 2021
@distantnative distantnative requested a review from lukasbestle June 5, 2021 18:20
lukasbestle
lukasbestle reviewed Jun 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I've fixed a few typos and made some changes to make the message more clear.

Could you please check why the E2E tests started failing? In one of the test cases an error component pops up, so it looks like we use the deprecated behavior ourselves somewhere.

Comment thread src/Cms/File.php Outdated
@distantnative
Copy link
Copy Markdown
Member Author

distantnative commented Jun 5, 2021

👍

lukasbestle
lukasbestle previously approved these changes Jun 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also updated the backend tests to pass the parent prop so we don't have this work when we make it required in 3.7.0.

@lukasbestle lukasbestle mentioned this pull request Jun 7, 2021
Merged
@lukasbestle lukasbestle force-pushed the fix/file-create-parent-optional branch from 5e5371d to ab3c64a Compare June 7, 2021 09:55
@lukasbestle
Copy link
Copy Markdown
Member

lukasbestle commented Jun 7, 2021

I've rebased this PR against develop to incorporate the CI Codecov fix. Besides the updated setParent() docblock there are no other changes.

@distantnative
Copy link
Copy Markdown
Member Author

distantnative commented Jun 7, 2021

👍 lgtm

@lukasbestle lukasbestle merged commit 12b38c2 into develop Jun 7, 2021
@lukasbestle lukasbestle deleted the fix/file-create-parent-optional branch June 7, 2021 10:44
@lukasbestle lukasbestle modified the milestones: 3.6.0, 3.6.0-alpha Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@lukasbestle lukasbestle lukasbestle approved these changes

Assignees

@distantnative distantnative

Labels

None yet

Milestone

3.6.0-alpha.1

Development

Successfully merging this pull request may close these issues.

3 participants

@distantnative @lukasbestle @bastianallgeier