Skip to content

Wireguard: fix PHP error on import#2400

Merged
limetech merged 1 commit into
unraid:masterfrom
bergware:master
Sep 24, 2025
Merged

Wireguard: fix PHP error on import#2400
limetech merged 1 commit into
unraid:masterfrom
bergware:master

Conversation

@bergware

@bergware bergware commented Sep 24, 2025

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Refactor

    • Streamlined how AllowedIPs are processed during WireGuard tunnel import/update, reusing computed values for consistent filtering and assignment.
    • Reduced redundant lookups to enhance maintainability and reliability.
  • Bug Fixes

    • Improved consistency in applying AllowedIPs across tunnels during updates/imports, reducing chances of mismatched filtering or subnet assignment.

@coderabbitai

coderabbitai Bot commented Sep 24, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

The WireGuard update import path now assigns AllowedIPs to a local variable ($allowedIPs), uses it when calling ipfilter, and sets subnets1 via "AllowedIPs=" concatenation with that variable. This replaces direct inline retrieval via _var(...). The diff indicates a potential missing semicolon and an extra closing parenthesis in the assignment.

Changes

Cohort / File(s) Change Summary
WireGuard import handling refactor
emhttp/plugins/dynamix/include/update.wireguard.php
Introduced local $allowedIPs to capture AllowedIPs; passed it to ipfilter and used it to build subnets1 string. Replaced inline _var('AllowedIPs') usage. Diff suggests a possible missing semicolon and an extra ) in the new assignment line.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my ears at lines that change,
A burrowed var now in exchange—
AllowedIPs, we hop it through,
To filters, strings, the tidy queue.
Mind the semis; parentheses too—
A rabbit reviews, then thumbscrew: 🥕✔️

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and succinctly highlights the primary change—resolving a PHP error encountered during WireGuard import—directly reflecting the core fix implemented in the changeset without extraneous detail.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown

🔧 PR Test Plugin Available

A test plugin has been generated for this PR that includes the modified files.

Version: 2025.09.24.1657
Build: View Workflow Run

📥 Installation Instructions:

Install via Unraid Web UI:

  1. Go to Plugins → Install Plugin
  2. Copy and paste this URL:
https://preview.dl.unraid.net/pr-plugins/pr-2400/webgui-pr-2400.plg
  1. Click Install

Alternative: Direct Download

⚠️ Important Notes:

  • Testing only: This plugin is for testing PR changes
  • Backup included: Original files are automatically backed up
  • Easy removal: Files are restored when plugin is removed
  • Conflicts: Remove this plugin before installing production updates

📝 Modified Files:

Click to expand file list
emhttp/plugins/dynamix/include/update.wireguard.php

🔄 To Remove:

Navigate to Plugins → Installed Plugins and remove webgui-pr-2400, or run:

plugin remove webgui-pr-2400

🤖 This comment is automatically generated and will be updated with each new push to this PR.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a22248 and 3facf57.

📒 Files selected for processing (1)
  • emhttp/plugins/dynamix/include/update.wireguard.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/update.wireguard.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
  • _var (106-108)

Comment on lines +544 to 547
$allowedIPs = _var($import,"AllowedIPs:$n")
ipfilter($allowedIPs);
if (_var($import,"TYPE:$n") == 0) $var['subnets1'] = "AllowedIPs=".$allowedIPs);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix syntax errors and wrong subnets mapping (TYPE 0 should use subnets2).

  • Missing semicolon after $allowedIPs = ...
  • Extra trailing ) at end of assignment
  • Logic bug: for TYPE == 0 (even), parseInput expects subnets2, not subnets1

Apply this diff:

-    $allowedIPs = _var($import,"AllowedIPs:$n")
-    ipfilter($allowedIPs);
-    if (_var($import,"TYPE:$n") == 0) $var['subnets1'] = "AllowedIPs=".$allowedIPs);
+    $allowedIPs = _var($import, "AllowedIPs:$n");
+    ipfilter($allowedIPs);
+    if ($vpn == 0 && $allowedIPs !== '') {
+      $var['subnets2'] = "AllowedIPs=".$allowedIPs;
+    }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In emhttp/plugins/dynamix/include/update.wireguard.php around lines 544-547: fix
the syntax and mapping bug by adding the missing semicolon after the $allowedIPs
assignment, remove the extra trailing parenthesis on the assignment line, and
change the mapping so that when _var($import,"TYPE:$n") == 0 you assign to
$var['subnets2'] (not subnets1); keep the ipfilter($allowedIPs) call as-is and
ensure the assignment uses the sanitized $allowedIPs variable.

@limetech limetech merged commit ad09b2b into unraid:master Sep 24, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants