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(memcache): remove double $$ to fix error #44794

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

Velwark
Copy link
Contributor

@Velwark Velwark commented Apr 11, 2024

Extra Dollar Sign caused errors in Nextcloud. Removing the Dollar Sign Solved the Problem.

  • Resolves: #

Summary

TODO

  • ...

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label Apr 11, 2024
@solracsf solracsf modified the milestones: Nextcloud 29, Nextcloud 30 Apr 11, 2024
@solracsf
Copy link
Member

/backport to stable29

@solracsf
Copy link
Member

/backport to stable28

@solracsf
Copy link
Member

/backport to stable27

@solracsf solracsf changed the title Update LoggerWrapperCache.php fix(memcache): remove double $$ to fix error Apr 14, 2024
@kesselb kesselb requested review from susnux and come-nc April 15, 2024 20:59
@come-nc
Copy link
Contributor

come-nc commented Apr 16, 2024

Nice catch.
This made search for $$ in our codebase, I think

$$groupBys = $groupBys[0];
is also a typo, no?

@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 16, 2024
@Velwark
Copy link
Contributor Author

Velwark commented Apr 16, 2024

Nice catch. This made search for $$ in our codebase, I think

$$groupBys = $groupBys[0];

is also a typo, no?

I‘m really no PHP expert myself. The Double Dollar Sign is a Variable Variable in PHP. As Long as ist doesn‘t throw any erros that could just be right.

The Double Dollar sign here in the Commit caused lots of errors on my Instance.

By the way, how do i merge this commit?

@come-nc
Copy link
Contributor

come-nc commented Apr 16, 2024

Nice catch. This made search for $$ in our codebase, I think

$$groupBys = $groupBys[0];

is also a typo, no?

I‘m really no PHP expert myself. The Double Dollar Sign is a Variable Variable in PHP. As Long as ist doesn‘t throw any erros that could just be right.

I think $groupBys is an array there, so it makes no sense. I’ll try to find time to debug&fix.

The Double Dollar sign here in the Commit caused lots of errors on my Instance.

Note that I think that means you have profiling enabled, you should disable it outside of dev instances. Check with occ config:system:get profiler.

By the way, how do i merge this commit?

You do not, we need to wait for someone with the power to do so to force merge, because the CI cannot pass on PRs coming from forks.

Extra Dollar Sign caused errors in Nextcloud. Removing the Dollar Sign Solved the Problem.

Signed-off-by: Velwark <levinfrerich9@gmail.com>
@AndyScherzinger AndyScherzinger merged commit 9872755 into nextcloud:master Apr 23, 2024
91 of 94 checks passed
Copy link

welcome bot commented Apr 23, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@AndyScherzinger
Copy link
Member

Thanks for your contribution @Velwark 🎉

Highly appreciated 💯🙏🎉

@nickvergessen
Copy link
Member

Looking at

$groupBys = $groupBys[0];
it seems to be a bug in line 980

@come-nc
Copy link
Contributor

come-nc commented Apr 23, 2024

Looking at

$groupBys = $groupBys[0];

it seems to be a bug in line 980

fixed it in 6953d1f in the psalm bump PR which I intend to get merged at some point.

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@joshtrichards
Copy link
Member

The Double Dollar sign here in the Commit caused lots of errors on my Instance.

@Velwark What were the errors you were seeing/under what conditions? Asking since this PR wasn't connected to prior bug report and just to get it on record for searches/etc. Thanks!

@nickvergessen
Copy link
Member

Well it triggered a warning depending on PHP config because of the use of Undefined variable $300 etc.

@joshtrichards
Copy link
Member

👍 I just realized this is in the wrapper (so only when redis_log_file is configured).

Probably a less traveled code path. Explains why there weren't a bunch of related bug reports.

@blizzz blizzz mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants