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

fix: expect interface, not a specific implementation #38625

Merged
merged 5 commits into from
Jun 23, 2023

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jun 2, 2023

Summary

  • fixes a regression when deleting folders while music app was enabled, for a LazyRoot was passed to this method.

Checklist

@blizzz blizzz added bug 3. to review Waiting for reviews regression labels Jun 2, 2023
@blizzz blizzz added this to the Nextcloud 28 milestone Jun 2, 2023
@blizzz blizzz requested review from a team, ArtificialOwl, icewind1991 and nfebe and removed request for a team June 2, 2023 23:22
@szaimen szaimen closed this Jun 4, 2023
@szaimen szaimen reopened this Jun 4, 2023
@blizzz blizzz requested a review from come-nc June 8, 2023 20:56
@blizzz blizzz force-pushed the fix/noid/querysearchehelper-narrow-type branch from 1acc054 to a38b21c Compare June 10, 2023 18:20
lib/private/Files/Cache/QuerySearchHelper.php Outdated Show resolved Hide resolved
lib/private/Files/Node/Root.php Outdated Show resolved Hide resolved
lib/private/Files/FileInfo.php Outdated Show resolved Hide resolved
lib/private/Files/Cache/QuerySearchHelper.php Outdated Show resolved Hide resolved
@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 14, 2023
@JoshuaPettus
Copy link

Why is this slated for NC28? It's a regression that effects NC26 and 27.

@paulijar
Copy link
Contributor

It's a regression that effects NC26 and 27.

And also NC25.

@blizzz
Copy link
Member Author

blizzz commented Jun 15, 2023

Why is this slated for NC28? It's a regression that effects NC26 and 27.

For this PR is against the master branch. Backports would have the specific milestone of the next maintenance release assigned.

@blizzz blizzz force-pushed the fix/noid/querysearchehelper-narrow-type branch from a38b21c to ff23772 Compare June 15, 2023 21:34
@blizzz blizzz force-pushed the fix/noid/querysearchehelper-narrow-type branch from ff23772 to cde9c1d Compare June 15, 2023 21:42
@blizzz
Copy link
Member Author

blizzz commented Jun 15, 2023

ERROR: UndefinedInterfaceMethod - lib/private/Files/Node/Folder.php:347:24 - Method OCP\Files\IRootFolder::createNode does not exist (see https://psalm.dev/181)
                return [$this->root->createNode(

Wow, how did this even work, ever? createNode is protected.

To answer the question myself:

Objects of the same type will have access to each others private and protected members even though they are not the same instances. This is because the implementation specific details are already known when inside those objects.

phpdoc

@blizzz blizzz removed the 2. developing Work in progress label Jun 16, 2023
@blizzz blizzz added the 3. to review Waiting for reviews label Jun 16, 2023
@blizzz blizzz requested a review from come-nc June 16, 2023 22:22
@blizzz blizzz requested a review from marcelklehr June 19, 2023 09:40
blizzz added 5 commits June 21, 2023 16:53
- fixes a regression when deleting folders while music app was enabled,
  for a LazyRoot was passed to this method.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
createNode() is protected and used by Folder, but being an internal-only
method it shall not be exposed in the Folder or IRootFolder interface.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/noid/querysearchehelper-narrow-type branch from 0b32684 to 088a0ee Compare June 21, 2023 14:53
@blizzz blizzz requested a review from marcelklehr June 21, 2023 14:53
@blizzz blizzz merged commit 1751599 into master Jun 23, 2023
@blizzz blizzz deleted the fix/noid/querysearchehelper-narrow-type branch June 23, 2023 22:13
@blizzz
Copy link
Member Author

blizzz commented Jun 23, 2023

/backport to stable27

@blizzz
Copy link
Member Author

blizzz commented Jun 23, 2023

/backport to stable26

@blizzz
Copy link
Member Author

blizzz commented Jun 23, 2023

/backport to stable25

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@szaimen
Copy link
Contributor

szaimen commented Jun 23, 2023

/backport to stable25

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

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

Successfully merging this pull request may close these issues.

Can't delete folders while the app is enabled
7 participants