XOOPS Support Plugin for PhpStorm - #1
Merged
Merged
Conversation
Reviewer's GuideInitial implementation of the XOOPS Support PhpStorm/IntelliJ plugin: project detection, filesystem scanner + tool window, XOOPS-specific inspections with quick fixes, language-constant completion, live templates, module scaffolding actions, Gradle build, CI workflows, and project documentation. Sequence diagram for refreshing the XOOPS Support overview tool windowsequenceDiagram
actor Developer
participant ToolsMenu
participant RefreshXoopsOverviewAction
participant ToolWindowManager
participant XoopsToolWindowPanel
participant ProgressManager
participant XoopsProjectScanner
participant XoopsReportHtmlRenderer
Developer->>ToolsMenu: Select "Refresh XOOPS Overview"
ToolsMenu->>RefreshXoopsOverviewAction: actionPerformed(e)
RefreshXoopsOverviewAction->>ToolWindowManager: getToolWindow("XOOPS Support")
RefreshXoopsOverviewAction->>XoopsToolWindowPanel: panel.refresh()
XoopsToolWindowPanel->>ProgressManager: run(Task.Backgroundable)
ProgressManager->>XoopsProjectScanner: scan(Path.of(basePath))
XoopsProjectScanner-->>ProgressManager: XoopsProjectReport
ProgressManager->>XoopsReportHtmlRenderer: render(report)
XoopsReportHtmlRenderer-->>ProgressManager: html
ProgressManager->>ToolWindowManager: invokeLater(() -> applyReport(report, html))
ToolWindowManager->>XoopsToolWindowPanel: applyReport(report, html)
XoopsToolWindowPanel-->>Developer: Updated HTML overview + findings
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe change adds the XOOPS Support IntelliJ plugin foundation. It includes Gradle builds, CI and release workflows, XOOPS project scanning, inspections and quick fixes, module scaffolding, completion, tool-window reporting, plugin registration, and project documentation. ChangesXOOPS Support plugin
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Several quick-fix implementations (e.g. ReplaceRangeQuickFix/InsertBeforeOffsetQuickFix users) compute absolute character offsets from a regex over the full file text and then reuse those offsets when the user applies the fix; consider deriving ranges from PSI (or re-running the search at fix time) to avoid corrupt edits if the file has been modified between inspection and Alt+Enter.
- XoopsLanguageConstantCompletionContributor recomputes all language constants on every completion by scanning the index and reading file contents; introducing a per-project cache with invalidation on VFS/PSI changes would significantly reduce completion overhead on larger XOOPS trees.
- XoopsProjectScanner walks the filesystem (up to depth 12) and reads whole files into memory for every scan; consider adding more aggressive per-file/per-module finding caps or simple caching/throttling so the tool window refresh remains responsive on very large projects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several quick-fix implementations (e.g. ReplaceRangeQuickFix/InsertBeforeOffsetQuickFix users) compute absolute character offsets from a regex over the full file text and then reuse those offsets when the user applies the fix; consider deriving ranges from PSI (or re-running the search at fix time) to avoid corrupt edits if the file has been modified between inspection and Alt+Enter.
- XoopsLanguageConstantCompletionContributor recomputes all language constants on every completion by scanning the index and reading file contents; introducing a per-project cache with invalidation on VFS/PSI changes would significantly reduce completion overhead on larger XOOPS trees.
- XoopsProjectScanner walks the filesystem (up to depth 12) and reads whole files into memory for every scan; consider adding more aggressive per-file/per-module finding caps or simple caching/throttling so the tool window refresh remains responsive on very large projects.
## Individual Comments
### Comment 1
<location path="src/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.java" line_range="61-70" />
<code_context>
+ for (String name : collectConstants(project)) {
</code_context>
<issue_to_address>
**suggestion (performance):** Constant collection runs on every completion invocation without caching and scans PSI text via regex, which can be expensive.
Because collectConstants() runs in a read action on every completion and regex-parses multiple files, this can add noticeable latency on large projects. Please cache the discovered constants per project (e.g., via a project service or in-memory cache with file-change invalidation) and reuse that set, so completion only filters a precomputed list instead of re-scanning PSI each time.
Suggested implementation:
```java
// Use cached constants to avoid re-scanning PSI on every completion invocation
for (String name : XoopsLanguageConstantsCache.getInstance(project).getConstants()) {
if (result.getPrefixMatcher().prefixMatches(name)) {
result.addElement(
LookupElementBuilder.create(name)
.withTypeText("XOOPS lang", true)
.withPresentableText(name)
);
}
}
```
To fully implement the suggested caching and invalidation, you will also need to:
1. Introduce a project-level cache service, e.g. `org.xoops.support.completion.XoopsLanguageConstantsCache`:
- Implement it as a project service (`@Service(Service.Level.PROJECT)` or using `project.getService(...)` depending on your plugin SDK).
- Internally maintain a `Set<String>` of constants and a flag indicating whether the cache is valid.
- Provide:
```java
public static XoopsLanguageConstantsCache getInstance(Project project)
public Set<String> getConstants()
public void invalidate()
```
- In `getConstants()`, if the cache is invalid or empty, recompute the set by delegating to the existing `collectConstants(Project project)` logic (you can move that logic into the cache class or call a shared utility), then store it and return the cached set.
2. Wire file-change invalidation:
- Register a PSI or VFS listener (e.g. `PsiTreeChangeListener` or `VirtualFileListener`) in the cache service’s constructor or a dedicated initialization method.
- On relevant changes to XOOPS language files (create/modify/delete), call `invalidate()` so that the next completion invocation recomputes the constants.
3. Update imports in `XoopsLanguageConstantCompletionContributor.java`:
- Add `import org.xoops.support.completion.XoopsLanguageConstantsCache;`.
4. Optionally deprecate or restrict direct calls to `collectConstants(Project project)`:
- If `collectConstants` is currently a private method in `XoopsLanguageConstantCompletionContributor`, move its implementation into the cache service (e.g. `XoopsLanguageConstantsCache#rebuild()`), and have the cache service be the only code path that performs the expensive PSI+regex scan.
- Keep the contributor focused on completion behavior and delegate all constant discovery to the cache service.
These additional changes ensure constants are computed once per project and reused across completion invocations, with automatic invalidation when language files change, addressing the performance concerns raised in your comment.
</issue_to_address>
### Comment 2
<location path="src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java" line_range="209-218" />
<code_context>
+ }
+ String name = path.getFileName().toString().toLowerCase(Locale.ROOT);
+ if (name.endsWith(".php")) {
+ // Cap findings per file so large modules stay usable.
+ int before = findings.size();
+ addFirst(findings, content, path, RAW_REQUEST, "RAW_REQUEST",
+ "Avoid $_REQUEST; use a scoped Xmf\\Request API.");
+ addFirst(findings, content, path, QUERY_F, "DEPRECATED_QUERY_F",
+ "queryF() is deprecated; use query() for reads or exec() for writes.");
+ addFirst(findings, content, path, QUOTE_STRING, "DEPRECATED_QUOTE_STRING",
+ "quoteString() is deprecated; use quote().");
+ addFirst(findings, content, path, MUTATING_QUERY, "MUTATING_QUERY",
+ "Mutating SQL must use exec(), not query().");
+ if (findings.size() - before > 20) {
+ // already capped by addFirst (one each)
+ }
</code_context>
<issue_to_address>
**suggestion:** The findings-per-file cap logic is currently ineffective and could be simplified or made explicit.
`scanSourceFile()` computes `before` and checks `findings.size() - before > 20`, but the `if` body is empty and `addFirst()` only adds one finding per pattern, so this cap is never enforced. This makes the cap comment misleading. Either remove the unused `before`/20 check, or implement a real per‑file cap (e.g., stop scanning further patterns or short‑circuit once the limit is reached) so behavior matches the intent.
Suggested implementation:
```java
String content = readSmallFile(path).orElse(null);
if (content == null) {
return;
}
String name = path.getFileName().toString().toLowerCase(Locale.ROOT);
if (name.endsWith(".php")) {
// Cap findings per file so large modules stay usable.
int before = findings.size();
int maxPerFile = 20;
addFirst(findings, content, path, RAW_REQUEST, "RAW_REQUEST",
"Avoid $_REQUEST; use a scoped Xmf\\Request API.");
if (findings.size() - before >= maxPerFile) {
return;
}
addFirst(findings, content, path, QUERY_F, "DEPRECATED_QUERY_F",
"queryF() is deprecated; use query() for reads or exec() for writes.");
if (findings.size() - before >= maxPerFile) {
return;
}
addFirst(findings, content, path, QUOTE_STRING, "DEPRECATED_QUOTE_STRING",
"quoteString() is deprecated; use quote().");
if (findings.size() - before >= maxPerFile) {
return;
}
addFirst(findings, content, path, MUTATING_QUERY, "MUTATING_QUERY",
"Mutating SQL must use exec(), not query().");
if (findings.size() - before >= maxPerFile) {
return;
}
}
List<Path> moduleRoots = findModuleRoots(projectRoot, webRoot, standaloneModule);
```
The per-file cap is now effective for the patterns shown, but to make the cap fully consistent across all findings for a given PHP file, you should:
1. Identify any additional scans performed in `scanSourceFile()` after this block (loops or other `addFirst`/similar calls) and add a guard such as:
`if (findings.size() - before >= maxPerFile) { return; }`
before starting those scans.
2. If there are non-PHP scans in this method that should also be capped per file, consider hoisting `before`/`maxPerFile` outside the `name.endsWith(".php")` block or introducing a small helper like `boolean fileCapReached(int before, int maxPerFile, List<XoopsFinding> findings)` to reuse the logic cleanly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
src/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.java-54-54 (1)
54-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
Locale.ROOTfor case conversion.
prefix.toUpperCase()andvf.getPath().toLowerCase()use the default locale. Under the Turkish locale"_mi_"uppercases to"_Mİ_"and"/LANGUAGE/"lowercases to"/lançuage/"-style mismatches for dottedI. Both the prefix check and the/language/path filter then fail for those users.🐛 Proposed fix
- String upper = prefix.toUpperCase(); + String upper = prefix.toUpperCase(Locale.ROOT);- String path = vf.getPath().replace('\\', '/').toLowerCase(); + String path = vf.getPath().replace('\\', '/').toLowerCase(Locale.ROOT);Add
import java.util.Locale;.Also applies to: 93-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.java` at line 54, Update the case conversions in the completion contributor, including prefix handling and the `/language/` path filter, to use `Locale.ROOT` instead of the default locale; add the `java.util.Locale` import and apply it to both `toUpperCase` and `toLowerCase` calls.src/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.java-42-45 (1)
42-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winState the dirname rule in the error message and allow one-character names.
[a-z][a-z0-9_]{1,32}rejects a single-letter dirname and caps the length at 33. The dialog text promises "letters, numbers, underscore" but does not state the length rule or the leading-letter rule. The error text "Invalid dirname." does not tell the user what to correct.♻️ Proposed change
- if (!dirname.matches("[a-z][a-z0-9_]{1,32}")) { - Messages.showErrorDialog(project, "Invalid dirname.", "New XOOPS Module Stub"); + if (!dirname.matches("[a-z][a-z0-9_]{0,31}")) { + Messages.showErrorDialog( + project, + "Invalid dirname. Use 1-32 characters: start with a letter, then letters, digits, or underscore.", + "New XOOPS Module Stub" + ); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.java` around lines 42 - 45, Update the dirname validation in NewXoopsModuleStubAction to allow one-character lowercase names and enforce the intended maximum length, while preserving the lowercase-letter start and lowercase letters, digits, and underscores rule. Replace the generic “Invalid dirname.” text in the showErrorDialog call with a message that states the complete dirname requirements, including leading character and length constraints.src/main/resources/liveTemplates/Xoops.xml-27-32 (1)
27-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
xofetchdbproduces invalid PHP with its default value.The
xofetchtemplate wraps the variable in quotes:query("$SQL$"). Thexofetchdbtemplate does not:query($SQL$). Both templates use the same default value"SELECT 1", which resolves to the bare textSELECT 1.xofetchdbtherefore expands to$db->query(SELECT 1);, which does not parse.🐛 Proposed fix
<template name="xofetchdb" - value="$$result = $$db->query($SQL$);&`#10`;if (!$$db->isResultSet($$result) || !($$result instanceof \mysqli_result)) {&`#10`; return null;&`#10`;}&`#10`;$$row = $$db->fetchArray($$result);$END$" + value="$$result = $$db->query("$SQL$");&`#10`;if (!$$db->isResultSet($$result) || !($$result instanceof \mysqli_result)) {&`#10`; return null;&`#10`;}&`#10`;$$row = $$db->fetchArray($$result);$END$"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/liveTemplates/Xoops.xml` around lines 27 - 32, Update the xofetchdb template’s SQL argument to quote the SQL variable, matching xofetch, so its default value expands into a valid PHP string passed to query(). Preserve the existing result validation and fetchArray logic.src/main/java/org/xoops/support/settings/XoopsConfigurable.java-74-81 (1)
74-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
isModifiedreports true whencoreProfileis null.
resetselects"Auto"whens.coreProfileis null.isModifiedthen evaluates"Auto".equals(null), which is false, so the negation makesisModifiedreturn true. For every project with a freshXoopsSettingsState, the settings page shows unsaved changes and keeps Apply enabled with no user edit.getSelectedItem()can also be null, which makes line 79 throw.Compare the normalized values with
Objects.equals.🐛 Proposed fix
`@Override` public boolean isModified() { XoopsSettingsState s = XoopsSettingsState.getInstance(project); + String selectedProfile = String.valueOf(profileBox.getSelectedItem()); + String storedProfile = s.coreProfile == null ? "Auto" : s.coreProfile; return enabledBox.isSelected() != s.enabled || suppressNotifyBox.isSelected() != s.suppressStartupNotification - || !profileBox.getSelectedItem().equals(s.coreProfile) + || !selectedProfile.equals(storedProfile) || !prefixField.getText().trim().equals(s.tablePrefix == null ? "" : s.tablePrefix); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/settings/XoopsConfigurable.java` around lines 74 - 81, Update isModified to normalize the selected profile and s.coreProfile consistently with reset, treating a null coreProfile as "Auto", and compare them with Objects.equals. Also handle a null profileBox.getSelectedItem() safely while preserving the existing enabled, notification, and prefix comparisons.src/main/resources/META-INF/plugin.xml-42-46 (1)
42-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace
pluginIcon.svgwith a tool-window icon. The current resource is 40×40. Use the required 13×13, 20×20, or 16×16 size for the target UI mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/META-INF/plugin.xml` around lines 42 - 46, Update the icon referenced by the XoopsToolWindowFactory toolWindow declaration from pluginIcon.svg to a dedicated tool-window icon resource sized 13×13, 20×20, or 16×16 as required by the target UI mode.src/main/resources/liveTemplates/Xoops.xml-72-73 (1)
72-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a quoted literal for
xocriteria.VALUE.
defaultValueaccepts variable names or quoted literals. The current$$uidis not a valid literal default. Use"$uid"to insert the PHP variable$uid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/liveTemplates/Xoops.xml` around lines 72 - 73, Update the VALUE variable definition in the Xoops live template to use the quoted literal defaultValue `"$uid"` instead of `$$uid`, preserving the intended insertion of the PHP `$uid` variable.GITHUB_SETUP.md-16-16 (1)
16-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the hard-coded nested checkout path.
The canonical repository is
XOOPS/phpstorm-plugin, but this command assumes a checkout underdocs/phpStormPlugins/xoops-support. A user who follows the documented clone flow can receive a path error. Run the commands from the repository root or use an explicit path placeholder.Proposed fix
- cd docs/phpStormPlugins/xoops-support # or your checkout of this tree + # Run the following commands from the root of your checkout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GITHUB_SETUP.md` at line 16, Update the checkout-directory command in GITHUB_SETUP.md to avoid the hard-coded docs/phpStormPlugins/xoops-support path; instruct users to run it from the repository root or use an explicit checkout-path placeholder while preserving the setup flow.TUTORIAL.md-76-80 (1)
76-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the Gradle wrapper in the PowerShell example.
The repository supplies
gradlew.bat. Use it instead of requiring a global Gradle installation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TUTORIAL.md` around lines 76 - 80, Update the PowerShell Gradle commands in TUTORIAL.md to invoke the repository-provided gradlew.bat wrapper instead of the global gradle executable, preserving the existing runIde and buildPlugin --continuous tasks.TUTORIAL.md-22-22 (1)
22-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a real XOOPS checkout in the detection example. This repository has no XOOPS root markers, so
XoopsProjectScanner.scanreturnsxoopsProject=false. Point users to a real XOOPS checkout or add a fixture with the required markers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TUTORIAL.md` at line 22, Update the detection example in TUTORIAL.md to use a genuine XOOPS checkout containing the root markers required by XoopsProjectScanner.scan, or add and reference a fixture that provides those markers so the example reports xoopsProject=true.GITHUB_SETUP.md-58-60 (1)
58-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the Marketplace publishing DSL.
For plugin version
2.18.1, configure the token insideintellijPlatform { publishing { ... } }, for example:
intellijPlatform { publishing { token = providers.environmentVariable("PUBLISH_TOKEN") } }
publishPlugin { token = … }is not the matching top-level DSL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GITHUB_SETUP.md` around lines 58 - 60, Update the build.gradle.kts guidance in the publishing setup steps to configure PUBLISH_TOKEN through the intellijPlatform { publishing { ... } } DSL, using the environment variable provider, and remove the incorrect top-level publishPlugin configuration.src/main/resources/inspectionDescriptions/XoopsMissingRegisteredTemplate.html-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the HTML5 doctype.
HTMLHint reports
doctype-firstfor this file. Insert<!doctype html>before<html>.Proposed fix
+<!doctype html> <html>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/inspectionDescriptions/XoopsMissingRegisteredTemplate.html` at line 1, Insert the HTML5 doctype declaration <!doctype html> immediately before the <html> element in XoopsMissingRegisteredTemplate.html, ensuring it is the first document content.Source: Linters/SAST tools
src/main/java/org/xoops/support/XoopsProjectService.java-58-66 (1)
58-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
Locale.ROOTfor the path lowercasing.
toLowerCase()uses the default locale. In a Turkish locale"/DOCS/"lowercases to"/docs/"with a dotlessı, so the vendor and docs filters stop working. Line 85 has the same problem.XoopsProjectScanneralready usesLocale.ROOTfor the same purpose.🐛 Proposed fix
- String path = file.getPath().replace('\\', '/').toLowerCase(); + String path = file.getPath().replace('\\', '/').toLowerCase(Locale.ROOT);Apply the same change on line 85 and add
import java.util.Locale;.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/XoopsProjectService.java` around lines 58 - 66, Update the path lowercasing in the file-selection logic and the corresponding lowercasing at line 85 to use Locale.ROOT, and add the java.util.Locale import. Preserve the existing vendor, docs, and node_modules filtering behavior.test-fixtures/bad_module_sample.php-3-6 (1)
3-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the fixture comment with the plugin name.
The comment says "XOOPS Grok inspections" and the sample path is
_grok_demo. The plugin is named "XOOPS Support" in the PR objectives, the notification text, and the tool window. Use one name.✏️ Proposed fix
-/** - * Sample file for manually verifying XOOPS Grok inspections + Alt+Enter quick fixes. - * Copy under a path containing /modules/ (e.g. htdocs/modules/_grok_demo/) and re-inspect. +/** + * Sample file for manually verifying XOOPS Support inspections + Alt+Enter quick fixes. + * Copy under a path containing /modules/ (e.g. htdocs/modules/_xoops_support_demo/) and re-inspect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-fixtures/bad_module_sample.php` around lines 3 - 6, Update the descriptive comment in bad_module_sample.php to consistently use the plugin name “XOOPS Support” instead of “XOOPS Grok,” including the sample module path if needed, while preserving the existing fixture instructions.src/main/java/org/xoops/support/XoopsStartupActivity.java-18-18 (1)
18-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the obsolete
StartupActivityimplementationThe plugin targets PhpStorm
2024.3.5(build243). This platform marksStartupActivityas@Obsoleteand recommendsProjectActivity. Migrate the activity and implement its suspendingexecute(Project)method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/XoopsStartupActivity.java` at line 18, Replace the obsolete StartupActivity implementation in XoopsStartupActivity with ProjectActivity, updating the class declaration and imports accordingly. Implement ProjectActivity’s suspending execute(Project) method while preserving the activity’s existing startup behavior.src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java-249-262 (1)
249-262: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReject absolute and traversal paths before the existence check.
XoopsProjectScannercan inspect files outsidemoduleRoot, which can suppress aMISSING_REGISTERED_TEMPLATEfinding.CreateMissingTemplateQuickFixrejects invalid..VFS child names and does not create files outside the module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java` around lines 249 - 262, Validate each extracted template path in the scanner’s matcher loop before resolving it against moduleRoot: reject absolute paths and any traversal components such as “..”. Treat rejected paths as missing and add the existing MISSING_REGISTERED_TEMPLATE finding, matching CreateMissingTemplateQuickFix’s invalid-path behavior, without performing filesystem checks outside moduleRoot.src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java-26-28 (1)
26-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
Locale.ROOTfor path and name lowercasing. Three inspections callString.toLowerCase()without a locale, so the result depends on the JVM default locale. Under a Turkish locale,Imaps toı, and a path segment such as/Includes/or a file namedTEMPLATE.TPLno longer matches the ASCII comparison.PhpTextUtilalready usesLocale.ROOTfor the same checks, so the two code paths disagree on the same file.
src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java#L26-L28: change.toLowerCase()to.toLowerCase(Locale.ROOT)on the virtual-file path and add thejava.util.Localeimport.src/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.java#L32-L34: change.toLowerCase()to.toLowerCase(Locale.ROOT)on the virtual-file path and add thejava.util.Localeimport.src/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java#L27-L27: changefile.getName().toLowerCase()tofile.getName().toLowerCase(Locale.ROOT)and add thejava.util.Localeimport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java` around lines 26 - 28, Use Locale.ROOT for all inspection path and filename lowercasing: update XoopsRootPathGuardInspection.java lines 26-28, XoopsSuperglobalInspection.java lines 32-34, and XoopsWrongSmartyDelimiterInspection.java line 27, adding the java.util.Locale import in each file.
🧹 Nitpick comments (19)
src/main/java/org/xoops/support/settings/XoopsConfigurable.java (1)
92-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
disposeUIResourcesto release the Swing components.
createComponentstores five component references in fields.Configurableimplementations should clear them indisposeUIResourcesso the settings page does not retain the UI after it closes.♻️ Proposed addition
`@Override` public void disposeUIResources() { panel = null; enabledBox = null; suppressNotifyBox = null; profileBox = null; prefixField = null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/settings/XoopsConfigurable.java` around lines 92 - 99, Add an overridden disposeUIResources() method to XoopsConfigurable that clears the five UI component fields—panel, enabledBox, suppressNotifyBox, profileBox, and prefixField—so the settings UI can be released after closing.src/main/java/org/xoops/support/ui/XoopsToolWindowPanel.java (1)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the caught exception and log it.
The parameter is named
ignored, but the block sets the status text. Rename it toexand log it at debug level so a malformed finding link can be diagnosed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/ui/XoopsToolWindowPanel.java` around lines 106 - 108, In the exception handler around opening the selected finding, rename the caught exception from ignored to ex and log ex at debug level before setting the existing failure status text. Keep the current status message and control flow unchanged.src/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.java (2)
112-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport the failure type and clean up the partial scaffold.
If
writeorcreateChildDirectoryfails midway, the module directory stays on disk with a partial file set. The catch block also showsex.getMessage(), which isnullfor several runtime exceptions, so the dialog can be empty. Delete the created directory on failure and use a non-null message fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.java` around lines 112 - 114, Update the exception handler in NewXoopsModuleStubAction to delete the newly created module directory when write or createChildDirectory fails, and show a non-null error message by falling back to the exception type when ex.getMessage() is null or empty. Preserve the existing dialog title and failure flow.
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the PHP namespace segment with StudlyCase.
capuppercases only the first character. For dirnamemy_module,capbecomesMy_module. The generated composer PSR-4 key becomesXoopsModules\My_module\and the class becomesMy_moduleService. The code is valid PHP, but it does not match PSR-4/PSR-12 naming used by XOOPS modules.♻️ Proposed helper
private static String studly(String dirname) { StringBuilder sb = new StringBuilder(dirname.length()); for (String part : dirname.split("_")) { if (!part.isEmpty()) { sb.append(Character.toUpperCase(part.charAt(0))).append(part.substring(1)); } } return sb.toString(); }Also applies to: 202-215, 218-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.java` at line 79, Replace the first-character-only capitalization assigned to cap with StudlyCase conversion for the dirname, using a helper near the action implementation that splits underscore-delimited parts, skips empty parts, and capitalizes each part. Ensure all downstream namespace, Composer PSR-4, and generated class-name uses of cap—including the referenced later blocks—use the converted value.src/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.java (1)
55-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the dead prefix guard.
If
upperstarts with_M,_A, or_C, the outer condition is false and the block does nothing. If it does not, the inner condition only checks for a leading underscore. The whole block is therefore equivalent to a single check for a leading underscore.♻️ Proposed simplification
- String upper = prefix.toUpperCase(Locale.ROOT); - if (!(upper.startsWith("_M") || upper.startsWith("_A") || upper.startsWith("_C"))) { - // Still allow mid-typing _MI etc. - if (!prefix.startsWith("_")) { - return; - } - } + if (!prefix.startsWith("_")) { + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.java` around lines 55 - 60, In the completion filtering logic, simplify the nested condition around upper and prefix so it directly returns when prefix does not start with an underscore. Remove the redundant startsWith checks for "_M", "_A", and "_C", preserving the existing behavior for underscore-prefixed input.src/main/resources/META-INF/plugin.xml (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the hardcoded
<version>element.The build uses the IntelliJ Platform Gradle Plugin, which writes the version into
plugin.xmlthroughpatchPluginXml. A literal<version>here duplicates the Gradleversionproperty and will drift from it on the next release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/META-INF/plugin.xml` at line 6, Remove the hardcoded <version> element from plugin.xml and rely on the IntelliJ Platform Gradle Plugin's patchPluginXml configuration to inject the project version, keeping the Gradle version property as the single source of truth.</code>test-fixtures/bad_module_sample.php (1)
11-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExclude this fixture from static analysis.
The file intentionally contains SQL injection, a dynamic include, and an undefined constant. The scanners in CI report all three on every run. Add
test-fixtures/to the ignore configuration of the PHP scanners so the signal stays useful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-fixtures/bad_module_sample.php` around lines 11 - 21, Add test-fixtures/ to the ignore or exclusion configuration used by the PHP static-analysis scanners, ensuring bad_module_sample.php and other intentional fixtures are skipped while normal source files remain analyzed.Source: Linters/SAST tools
src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java (4)
105-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSurface the I/O failure instead of returning an empty module list.
If
Files.listfails, the report shows zero modules and no error.scanModulerecords aSCAN_ERRORfinding for the same class of failure. Make the behavior consistent, for example by returning the error toscanso it can add aSCAN_ERRORfinding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java` around lines 105 - 113, Update the module-directory listing flow in the scanner method containing Files.list so IOException is not silently converted to List.of(). Propagate the failure to scan, following scanModule’s existing SCAN_ERROR handling pattern so the report records a SCAN_ERROR finding instead of reporting zero modules.
174-181: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrune excluded directories during traversal.
Files.walkenumerates every entry beforeisExcludedfilters it. The scanner therefore descends fully intovendor,node_modules,uploads, andtemplates_c. On a real XOOPS tree this dominates the scan time. UseFiles.walkFileTreewith aFileVisitorthat returnsFileVisitResult.SKIP_SUBTREEfor excluded directory names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java` around lines 174 - 181, Replace the Files.walk pipeline in the scanner method with Files.walkFileTree and a FileVisitor that checks directory paths against the existing isExcluded logic, returning FileVisitResult.SKIP_SUBTREE for excluded directories such as vendor, node_modules, uploads, and templates_c. Preserve the current depth limit, regular-file filtering, PHP/TPL extension matching, and scanSourceFile invocation for included files.
48-80: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a cancellation hook for long scans.
scanwalks the whole module tree with no cancellation check.XoopsToolWindowPanelruns it inside a background task with aProgressIndicator, but the user cannot cancel a scan of a large XOOPS tree. Accept an optionalBooleanSupplierorRunnablecancellation callback and check it in theFiles.walkloops.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java` around lines 48 - 80, Update XoopsProjectScanner.scan and the module-tree scanning flow to accept an optional cancellation callback, then check it during each Files.walk iteration and stop promptly when cancellation is requested. Thread the callback through the relevant scanModule/findings methods and have XoopsToolWindowPanel connect it to its ProgressIndicator cancellation state while preserving existing behavior when no callback is supplied.
209-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the no-op cap block.
beforeis only read inside an emptyifbody.addFirstalready adds at most one finding per pattern, so the cap check has no effect.♻️ Proposed cleanup
if (name.endsWith(".php")) { - // Cap findings per file so large modules stay usable. - int before = findings.size(); addFirst(findings, content, path, RAW_REQUEST, "RAW_REQUEST", "Avoid $_REQUEST; use a scoped Xmf\\Request API."); addFirst(findings, content, path, QUERY_F, "DEPRECATED_QUERY_F", "queryF() is deprecated; use query() for reads or exec() for writes."); addFirst(findings, content, path, QUOTE_STRING, "DEPRECATED_QUOTE_STRING", "quoteString() is deprecated; use quote()."); addFirst(findings, content, path, MUTATING_QUERY, "MUTATING_QUERY", "Mutating SQL must use exec(), not query()."); - if (findings.size() - before > 20) { - // already capped by addFirst (one each) - } } else if (name.endsWith(".tpl")) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java` around lines 209 - 221, Remove the unused before variable and the empty findings.size() cap-check block from the finding collection logic in XoopsProjectScanner, while preserving all four addFirst calls and their existing behavior.src/main/java/org/xoops/support/inspections/PhpTextUtil.java (1)
35-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant third clause.
The first clause already matches every path that contains
/language/. The clausepath.endsWith("/main.php") && path.contains("/language/")can never add a match. The mixed||/&&also hides the intent, because&&binds tighter than||.If the intent is to skip language
main.phpfiles only, the first clause makes that unnecessary. If the intent is to skip anymain.php, state that explicitly.♻️ Proposed simplification
return path.contains("/language/") - || path.endsWith("/modinfo.php") - || path.endsWith("/main.php") && path.contains("/language/"); + || path.endsWith("/modinfo.php");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/PhpTextUtil.java` around lines 35 - 38, In the path-matching method, remove the redundant `path.endsWith("/main.php") && path.contains("/language/")` clause, leaving the existing `/language/` and `modinfo.php` checks unchanged.src/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.java (2)
59-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid copying the file tail for every match.
text.substring(m.start())allocates a new string from the match position to the end of the file. For a file with manyqueryF/quoteStringoccurrences this is quadratic in file size. Inspections run on the fly for each edit, so this cost repeats.Use a matcher region on the original text instead.
⚡ Proposed fix
- Matcher sqlMatch = QUERY_F_SQL.matcher(text.substring(m.start())); + Matcher sqlMatch = QUERY_F_SQL.matcher(text).region(m.start(), text.length());
regionkeepslookingAt()anchored atm.start(), so the classification logic is unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.java` around lines 59 - 62, Update the SQL classification logic around QUERY_F_SQL and sqlMatch to match against the original text using a matcher region beginning at m.start(), then call lookingAt() without creating text.substring(m.start()). Preserve the existing classification behavior and anchoring at the match position.
77-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the identical branches.
The
knownSelectbranch and theelsebranch run the same statement with the same fix order. PMD reports this asIdenticalConditionalBranches. Keep the classification only where it changes behavior.♻️ Proposed simplification
if (knownMutation) { holder.registerProblem(leaf, message, toExec, toQuery); - } else if (knownSelect) { - holder.registerProblem(leaf, message, toQuery, toExec); } else { - // Variable SQL or non-literal: offer both (common peer-plugin behavior) + // Known SELECT, variable SQL, or non-literal: offer both, query() first. holder.registerProblem(leaf, message, toQuery, toExec); }Then remove the now-unused
knownSelectflag and itselse ifcomputation at lines 61 and 68-71.If the intent is a different message for known SELECT statements, add that message instead of removing the branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.java` around lines 77 - 84, In XoopsDeprecatedDbApiInspection, merge the knownSelect and fallback branches so both use the existing toQuery, toExec registration order, while preserving the distinct knownMutation order. Remove the now-unused knownSelect declaration and its computation, unless a distinct known-SELECT message is intentionally required.Source: Linters/SAST tools
src/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe guard condition is always true.
The regex anchors the match at
{and ends it at}, and the(?<!<)lookbehind rejects a preceding<. Sooriginalalways starts with{, always ends with}, and never starts with<{. The condition adds no filtering.Related: in the pattern at line 19, the alternatives
/ifand/foreachare unreachable, because the preceding/?already consumes the slash.♻️ Proposed cleanup
private static final Pattern WRONG_SMARTY = Pattern.compile( - "(?i)(?<!<)\\{(?:\\$|/?(?:if|foreach|include|assign|block|literal|/if|/foreach)\\b)[^\\n}]*\\}" + "(?i)(?<!<)\\{(?:\\$|/?(?:if|foreach|include|assign|block|literal)\\b)[^\\n}]*\\}" );String original = m.group(); // {if $x} -> <{if $x}> | {$foo} -> <{$foo}> String fixed = "<" + original.substring(0, original.length() - 1) + "}>"; - if (original.startsWith("{") && original.endsWith("}") && !original.startsWith("<{")) { - holder.registerProblem( - leaf, - "XOOPS: Smarty tags should use <{ ... }> delimiters, not bare { ... }", - new ReplaceRangeQuickFix( - "Convert to XOOPS <{ }> delimiters", - m.start(), - m.end(), - fixed - ) - ); - count++; - } + holder.registerProblem( + leaf, + "XOOPS: Smarty tags should use <{ ... }> delimiters, not bare { ... }", + new ReplaceRangeQuickFix( + "Convert to XOOPS <{ }> delimiters", + m.start(), + m.end(), + fixed + ) + ); + count++;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java` at line 45, Remove the redundant startsWith/endsWith/startsWith guard around the matched value in XoopsWrongSmartyDelimiterInspection, since the regex already enforces those conditions. Also update the regex alternatives so /if and /foreach remain reachable instead of being shadowed by the preceding optional slash pattern.src/main/java/org/xoops/support/inspections/XoopsMissingRegisteredTemplateInspection.java (1)
27-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the vendor/cache exclusion for consistency.
Every other inspection in this cohort calls
PhpTextUtil.looksLikeVendorOrCache(file)before it reports. This inspection does not. A vendored copy of a module under/vendor/that ships axoops_version.phpwill produce findings that the user cannot act on.♻️ Proposed fix
if (!"xoops_version.php".equalsIgnoreCase(file.getName())) { return; } + if (PhpTextUtil.looksLikeVendorOrCache(file)) { + return; + } VirtualFile vf = file.getVirtualFile();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsMissingRegisteredTemplateInspection.java` around lines 27 - 34, Update visitFile in XoopsMissingRegisteredTemplateInspection to return immediately when PhpTextUtil.looksLikeVendorOrCache(file) identifies the file as vendor or cache content, before processing xoops_version.php. Preserve the existing filename and virtual-file checks for non-excluded files.src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java (2)
47-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe generated guard assumes the enclosing function returns null.
The fix inserts
return null;. The fetch call can sit in avoidfunction, in a constructor, in a loop wherecontinueis correct, or at file scope wherereturnends the included file. In those cases the applied fix changes control flow or conflicts with the declared return type.Consider a body that is safe in every scope, and let the developer replace it.
♻️ Proposed change to the generated body
String block = indentGuess + "if (!" + dbExpr + "->isResultSet(" + resultVar + ") || !" + resultVar + " instanceof \\mysqli_result) {\n" - + indentGuess + " return null;\n" + + indentGuess + " // TODO: handle the failed query (return, continue, or throw).\n" + indentGuess + "}\n";The
!$x instanceof \mysqli_resultform is correct, becauseinstanceofbinds tighter than!in PHP.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java` around lines 47 - 50, Update the generated guard body in XoopsResultSetGuardInspection so it no longer hardcodes “return null;”, which is unsafe across functions, constructors, loops, and file scope. Generate a scope-neutral replacement body that the developer can customize, while preserving the existing isResultSet and mysqli_result condition.
34-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe 400-character window ignores the result variable.
The check suppresses the finding when the text
isResultSetappears anywhere in the preceding 400 characters. It matches a guard for a different variable, a guard in a different function, and the word inside a comment. It also misses a guard placed more than 400 characters earlier.Bind the window check to the captured result variable to reduce false negatives.
♻️ Proposed refinement
- int start = Math.max(0, m.start() - 400); - String window = text.substring(start, m.start()); - if (window.contains("isResultSet")) { - continue; - } PsiElement leaf = PhpTextUtil.leafAt(file, m.start()); if (leaf == null) { continue; } String dbExpr = m.group(1).replaceAll("\\s+", ""); String resultVar = m.group(3); + int start = Math.max(0, m.start() - 400); + String window = text.substring(start, m.start()).replaceAll("\\s+", ""); + if (window.contains("isResultSet(" + resultVar + ")")) { + continue; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java` around lines 34 - 38, Update the guard detection in XoopsResultSetGuardInspection to associate the preceding-text check with the captured result variable, rather than suppressing findings for any occurrence of “isResultSet” in the fixed 400-character window. Ensure guards for different variables, unrelated functions, or comments do not match, and retain support for valid guards located beyond the current window.src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java (1)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegistering the problem on the whole file highlights the entire file.
holder.registerProblem(file, ...)applies the highlight range to the completePsiFile. In the editor the user sees the whole file marked. Anchor the problem to the opening<?phpleaf, or pass an explicitTextRangeso only the file head is highlighted.PhpTextUtil.rangealready exists for that purpose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java` around lines 49 - 55, Update the registerProblem call in XoopsRootPathGuardInspection to anchor the reported problem to the opening <?php leaf or an explicit head TextRange instead of the entire PsiFile; reuse PhpTextUtil.range for the range calculation, while preserving the existing message and InsertRootPathGuardQuickFix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bfd8441-b893-475c-aed9-47581ee2c513
⛔ Files ignored due to path filters (2)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jarsrc/main/resources/META-INF/pluginIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (59)
.gitattributes.github/workflows/gradle.yml.github/workflows/release.yml.gitignoreCHANGELOG.mdCONTRIBUTING.mdGITHUB_SETUP.mdLICENSEREADME.mdTUTORIAL.mdbuild.gradle.ktsgradle.propertiesgradle/wrapper/gradle-wrapper.propertiesgradlewgradlew.batsettings.gradle.ktssrc/main/java/org/xoops/support/XoopsProjectService.javasrc/main/java/org/xoops/support/XoopsStartupActivity.javasrc/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.javasrc/main/java/org/xoops/support/actions/RefreshXoopsOverviewAction.javasrc/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.javasrc/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.javasrc/main/java/org/xoops/support/inspections/CreateMissingTemplateQuickFix.javasrc/main/java/org/xoops/support/inspections/DocumentEditHelper.javasrc/main/java/org/xoops/support/inspections/InsertBeforeOffsetQuickFix.javasrc/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.javasrc/main/java/org/xoops/support/inspections/PhpTextUtil.javasrc/main/java/org/xoops/support/inspections/ReplaceRangeQuickFix.javasrc/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.javasrc/main/java/org/xoops/support/inspections/XoopsIncludeOnceHeaderInspection.javasrc/main/java/org/xoops/support/inspections/XoopsMissingRegisteredTemplateInspection.javasrc/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.javasrc/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.javasrc/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.javasrc/main/java/org/xoops/support/scanner/CoreProfile.javasrc/main/java/org/xoops/support/scanner/XoopsFinding.javasrc/main/java/org/xoops/support/scanner/XoopsModuleInfo.javasrc/main/java/org/xoops/support/scanner/XoopsProjectReport.javasrc/main/java/org/xoops/support/scanner/XoopsProjectScanner.javasrc/main/java/org/xoops/support/settings/XoopsConfigurable.javasrc/main/java/org/xoops/support/settings/XoopsSettingsState.javasrc/main/java/org/xoops/support/ui/XoopsReportHtmlRenderer.javasrc/main/java/org/xoops/support/ui/XoopsToolWindowFactory.javasrc/main/java/org/xoops/support/ui/XoopsToolWindowPanel.javasrc/main/resources/META-INF/plugin.xmlsrc/main/resources/inspectionDescriptions/XoopsDeprecatedDbApi.htmlsrc/main/resources/inspectionDescriptions/XoopsIncludeOnceHeader.htmlsrc/main/resources/inspectionDescriptions/XoopsMissingRegisteredTemplate.htmlsrc/main/resources/inspectionDescriptions/XoopsQueryExec.htmlsrc/main/resources/inspectionDescriptions/XoopsResultSetGuard.htmlsrc/main/resources/inspectionDescriptions/XoopsRootPathGuard.htmlsrc/main/resources/inspectionDescriptions/XoopsSuperglobal.htmlsrc/main/resources/inspectionDescriptions/XoopsWrongSmartyDelimiter.htmlsrc/main/resources/liveTemplates/Xoops.xmltest-fixtures/bad_module_sample.phptest-fixtures/bad_template_sample.tplwhats-new.html
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.java`:
- Around line 41-43: Update the existing-guard detection in
InsertRootPathGuardQuickFix so it checks for the complete normalized expression
defined('XOOPS_ROOT_PATH') rather than independently searching for “defined” and
“xoops_root_path”. Preserve the early return only when that combined root-path
guard is present.
In
`@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java`:
- Around line 47-51: The generated guard in XoopsResultSetGuardInspection must
prevent execution from reaching the fetch when the result is invalid; generate a
scope-safe control-flow action for the enclosing context (such as return,
continue, or throw), or remove the automatic quick fix until that behavior can
be determined safely. Replace the comment-only body in the generated if branch
while preserving valid handling for constructors, void methods, and other
scopes.
- Around line 36-40: Replace the text-window check in
XoopsResultSetGuardInspection with PHP PSI control-flow analysis that identifies
an isResultSet(resultVar) condition in the same function and suppresses only
when that condition dominates the fetch call; do not match comments, unrelated
functions, or sibling branches. Update the quick-fix generation so the inserted
failure branch terminates or skips execution before fetchArray, fetchRow, or
fetchBoth.
In `@src/main/resources/liveTemplates/Xoops.xml`:
- Line 28: Update the xofetchdb template value so it never uses return;
initialize $row to null, then conditionally call fetchArray only when the query
result passes isResultSet and mysqli_result validation, preserving normal row
fetching while safely handling query failures.
In `@whats-new.html`:
- Line 1: Add <!doctype html> before the first element in whats-new.html,
placing it immediately before the existing <h3>1.0.0 Alpha 3</h3> heading.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8017d00-6f9e-4ec4-b5bb-cec263c575be
⛔ Files ignored due to path filters (1)
src/main/resources/icons/toolWindowXoops.svgis excluded by!**/*.svg
📒 Files selected for processing (41)
.github/workflows/gradle.yml.github/workflows/release.ymlCHANGELOG.mdCONTRIBUTING.mdGITHUB_SETUP.mdLICENSEREADME.mdTUTORIAL.mdbuild.gradle.ktsgradle.propertiessrc/main/java/org/xoops/support/XoopsProjectService.javasrc/main/java/org/xoops/support/XoopsStartupActivity.javasrc/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.javasrc/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.javasrc/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.javasrc/main/java/org/xoops/support/completion/XoopsLanguageConstantsCache.javasrc/main/java/org/xoops/support/inspections/DocumentEditHelper.javasrc/main/java/org/xoops/support/inspections/InsertBeforeOffsetQuickFix.javasrc/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.javasrc/main/java/org/xoops/support/inspections/ReplaceRangeQuickFix.javasrc/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.javasrc/main/java/org/xoops/support/inspections/XoopsIncludeOnceHeaderInspection.javasrc/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.javasrc/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.javasrc/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.javasrc/main/java/org/xoops/support/scanner/XoopsProjectScanner.javasrc/main/java/org/xoops/support/settings/XoopsConfigurable.javasrc/main/java/org/xoops/support/ui/XoopsToolWindowPanel.javasrc/main/resources/META-INF/plugin.xmlsrc/main/resources/inspectionDescriptions/XoopsDeprecatedDbApi.htmlsrc/main/resources/inspectionDescriptions/XoopsIncludeOnceHeader.htmlsrc/main/resources/inspectionDescriptions/XoopsMissingRegisteredTemplate.htmlsrc/main/resources/inspectionDescriptions/XoopsQueryExec.htmlsrc/main/resources/inspectionDescriptions/XoopsResultSetGuard.htmlsrc/main/resources/inspectionDescriptions/XoopsRootPathGuard.htmlsrc/main/resources/inspectionDescriptions/XoopsSuperglobal.htmlsrc/main/resources/inspectionDescriptions/XoopsWrongSmartyDelimiter.htmlsrc/main/resources/liveTemplates/Xoops.xmlwhats-new.html
🚧 Files skipped from review as they are similar to previous changes (28)
- src/main/resources/inspectionDescriptions/XoopsMissingRegisteredTemplate.html
- src/main/resources/inspectionDescriptions/XoopsQueryExec.html
- gradle.properties
- src/main/resources/inspectionDescriptions/XoopsDeprecatedDbApi.html
- src/main/resources/inspectionDescriptions/XoopsResultSetGuard.html
- src/main/resources/inspectionDescriptions/XoopsWrongSmartyDelimiter.html
- LICENSE
- src/main/resources/inspectionDescriptions/XoopsIncludeOnceHeader.html
- src/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.java
- src/main/resources/inspectionDescriptions/XoopsSuperglobal.html
- .github/workflows/gradle.yml
- src/main/resources/inspectionDescriptions/XoopsRootPathGuard.html
- src/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.java
- TUTORIAL.md
- build.gradle.kts
- src/main/java/org/xoops/support/settings/XoopsConfigurable.java
- src/main/java/org/xoops/support/ui/XoopsToolWindowPanel.java
- src/main/java/org/xoops/support/XoopsStartupActivity.java
- src/main/java/org/xoops/support/XoopsProjectService.java
- src/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java
- src/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.java
- src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java
- README.md
- GITHUB_SETUP.md
- src/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.java
- src/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.java
- src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java
- src/main/java/org/xoops/support/inspections/XoopsIncludeOnceHeaderInspection.java
Greptile SummaryThe PR introduces the initial XOOPS Support plugin for PhpStorm, including inspections, quick fixes, completion, project scanning, scaffolding, settings, documentation, and Gradle-based CI/release automation.
Confidence Score: 4/5The PR is not yet safe to merge because XOR-based result-set guards can still suppress diagnostics for fetches reachable with an invalid result. The positive-guard classifier recognizes disjunction only as Files Needing Attention: src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java
|
| Filename | Overview |
|---|---|
| src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java | Implements heuristic result-set guard detection and quick fixes, but the previously reported XOR guard misclassification remains. |
| .github/workflows/release.yml | Builds tagged releases with repository-write permission while pinning all third-party actions to immutable revisions. |
| src/main/resources/META-INF/plugin.xml | Registers the plugin services, actions, tool window, completion contributor, and XOOPS inspections. |
| src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java | Adds background XOOPS project and module scanning used by the overview tooling. |
Reviews (11): Last reviewed commit: "v1.0.0-alpha.1" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Suppression only if (a) an early-exit if (!isResultSet($var)) { return|throw|… } ends immediately before the fetch, or (b) fetch is still inside a positive if ( isResultSet($var)) { … } via brace-depth walk
- Never emit continue. Quick fix always uses throw new \RuntimeException('Database query failed'); (valid in methods, constructors, loops, file scope)
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java (1)
170-188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBind the Composer version to the XOOPS package entry.
The global
"xoops"check does not qualify the version match. Acomposer.jsonfile can contain an unrelated XOOPS package and an unrelated"^4.0"dependency.VERSION_40then reportsXOOPS_40.Match the package name and its version constraint as one Composer entry. Do not apply
VERSION_25,VERSION_27, orVERSION_40to the complete Composer file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java` around lines 170 - 188, The fallback logic in XoopsProjectScanner must bind the XOOPS package name to its version constraint instead of matching “xoops” and version patterns independently across the full Composer file. Update the candidate-processing code to extract or match a single Composer package entry, then apply VERSION_25, VERSION_27, or VERSION_40 only to that entry’s version constraint.src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.java (1)
48-62: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInsert the guard only at a valid file-entry PHP block.
OPEN_TAG.matcher(text).find()can match<?=or a PHP tag after emitted content. Insertingdefined(...) || exit(...)after<?=creates invalid PHP. Inserting after a later tag leaves earlier output unguarded.Resolve the opening PHP tag from PSI. Insert after a file-leading statement tag only. For a file-leading
<?=tag, insert a separate<?php ... ?>guard block before it, or do not offer this fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.java` around lines 48 - 62, Update the guard insertion logic in InsertRootPathGuardQuickFix so it resolves the opening PHP tag from PSI rather than using OPEN_TAG.matcher(text).find(). Only insert after a file-leading statement PHP tag; never insert into <?= tags or after a tag following emitted content. For a file-leading short echo tag, insert a separate valid <?php guard block before it, or decline the quick fix.
🧹 Nitpick comments (1)
settings.gradle.kts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfigure a Java toolchain or remove the resolver plugin.
build.gradle.ktsdoes not request a Java toolchain, so the Foojay resolver cannot provision JDK 21. Add a Java 21 toolchain declaration if automatic JDK provisioning is intended. Otherwise, remove the resolver plugin.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@settings.gradle.kts` around lines 1 - 4, Resolve the unused Foojay resolver configuration in the Gradle setup: either add a Java 21 toolchain declaration to the project’s Java configuration so automatic provisioning is used, or remove the org.gradle.toolchains.foojay-resolver-convention plugin from settings.gradle.kts if provisioning is not intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@GITHUB_SETUP.md`:
- Around line 15-24: Update the Git setup instructions around the “Run from the
root” preface to clarify that git init and git remote add require an
uninitialized standalone tree, not an existing clone. Alternatively, provide the
appropriate git remote set-url origin guidance for users working from a clone,
while preserving the existing initialization and push steps.
In `@README.md`:
- Line 17: Update the “Inspections + quick fixes” bullet in README.md to
distinguish keyed superglobal accesses, which XoopsSuperglobalInspection can
replace with \Xmf\Request, from bare $_GET/$_POST/$_REQUEST/$_COOKIE accesses,
which are warning-only and have no quick fix.
In `@src/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.java`:
- Around line 34-38: Update the error callback in the scan exception handler to
check project.isDisposed() before invoking Messages.showErrorDialog, matching
the guard used by the success callback and avoiding UI access after project
disposal.
In `@src/main/java/org/xoops/support/completion/XoopsLanguageConstantsCache.java`:
- Around line 45-46: Update XoopsLanguageConstantsCache so cache invalidation is
atomic and observes PSI modifications: replace the current AtomicReference
rebuild flow in getConstants() and invalidate() with a CachedValue keyed to
PsiModificationTracker.MODIFICATION_COUNT, or otherwise reject any rebuild whose
collection began before the invalidation generation. Ensure unsaved
PsiFile.getText() edits invalidate the cached constants and cannot be
republished after invalidate().
In
`@src/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.java`:
- Around line 60-86: Update the queryF()/query() to exec() quick-fix handling in
XoopsDeprecatedDbApiInspection.java:60-86 and
XoopsQueryExecInspection.java:52-57 so it only offers exec() for one-argument
calls, or removes optional $limit and $start arguments when converting
multi-argument calls. Preserve those arguments for query() replacements, and
ensure every exec() quick fix produces a valid call accepting only $sql.
In
`@src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java`:
- Around line 50-55: Update the guard-detection logic in
XoopsRootPathGuardInspection so it no longer suppresses findings based on any
ROOT_PATH_GUARD match anywhere in the raw file text. Parse or scan executable
top-level statements in source order, ignoring comments and string contents, and
suppress only when the first relevant guard is a terminating
defined('XOOPS_ROOT_PATH') check before other executable code; support both
defined(...) combined with || exit/die and if (!defined(...)) { exit/die; }
forms.
In `@src/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.java`:
- Around line 41-80: Update
src/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.java:41-80,
XoopsDeprecatedDbApiInspection.java:39-87, and
XoopsQueryExecInspection.java:33-58 to use PHP syntax-aware token matching,
excluding comments and string literals before registering rename or replacement
fixes. Update XoopsIncludeOnceHeaderInspection.java:32-52 to recognize only PHP
include statements, XoopsMissingRegisteredTemplateInspection.java:36-50 to parse
manifest assignments while ignoring comments, and
XoopsWrongSmartyDelimiterInspection.java:34-58 to skip Smarty comments and
<{literal}> content before offering delimiter fixes.
- Around line 54-80: Update the inspection logic around the keyed superglobal
quick-fix construction and holder.registerProblem so automatic replacements are
offered only when PSI analysis proves the value is a string and the source is
known. Remove unconditional getString() fixes, including all forced-source fixes
for $_REQUEST; when usage or source is ambiguous, report the problem with no
replacement and leave accessor selection to the developer. Preserve getArray()
handling for proven array values.
In `@src/main/resources/inspectionDescriptions/XoopsResultSetGuard.html`:
- Line 6: Update the `<pre>` content in XoopsResultSetGuard.html to HTML-escape
the object access operator, replacing the raw `->` in `$db->isResultSet` with
`->` while preserving the displayed PHP condition.
In `@src/main/resources/META-INF/plugin.xml`:
- Around line 60-68: Make the project-level enabled setting from
XoopsConfigurable control all registered inspections and completion providers.
Add or reuse a shared enabled-state check in XoopsRootPathGuardInspection and
XoopsLanguageConstantCompletionContributor, plus every other registered
provider, so they stop running when XOOPS Support is disabled while preserving
their current behavior when enabled.
In `@test-fixtures/bad_module_sample.php`:
- Around line 3-4: Update the fixture description comments to replace “XOOPS
Grok” with “XOOPS Support” and change the example module path suffix from
“_grok_demo” to “_xoops_demo,” keeping the manual inspection instructions
otherwise unchanged.
---
Duplicate comments:
In
`@src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.java`:
- Around line 48-62: Update the guard insertion logic in
InsertRootPathGuardQuickFix so it resolves the opening PHP tag from PSI rather
than using OPEN_TAG.matcher(text).find(). Only insert after a file-leading
statement PHP tag; never insert into <?= tags or after a tag following emitted
content. For a file-leading short echo tag, insert a separate valid <?php guard
block before it, or decline the quick fix.
In `@src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java`:
- Around line 170-188: The fallback logic in XoopsProjectScanner must bind the
XOOPS package name to its version constraint instead of matching “xoops” and
version patterns independently across the full Composer file. Update the
candidate-processing code to extract or match a single Composer package entry,
then apply VERSION_25, VERSION_27, or VERSION_40 only to that entry’s version
constraint.
---
Nitpick comments:
In `@settings.gradle.kts`:
- Around line 1-4: Resolve the unused Foojay resolver configuration in the
Gradle setup: either add a Java 21 toolchain declaration to the project’s Java
configuration so automatic provisioning is used, or remove the
org.gradle.toolchains.foojay-resolver-convention plugin from settings.gradle.kts
if provisioning is not intended.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 284b4df9-cece-41db-b408-5a03e227425e
⛔ Files ignored due to path filters (3)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jarsrc/main/resources/META-INF/pluginIcon.svgis excluded by!**/*.svgsrc/main/resources/icons/toolWindowXoops.svgis excluded by!**/*.svg
📒 Files selected for processing (60)
.gitattributes.github/workflows/gradle.yml.github/workflows/release.yml.gitignoreCHANGELOG.mdCONTRIBUTING.mdGITHUB_SETUP.mdLICENSEREADME.mdTUTORIAL.mdbuild.gradle.ktsgradle.propertiesgradle/wrapper/gradle-wrapper.propertiesgradlewgradlew.batsettings.gradle.ktssrc/main/java/org/xoops/support/XoopsProjectService.javasrc/main/java/org/xoops/support/XoopsStartupActivity.javasrc/main/java/org/xoops/support/actions/NewXoopsModuleStubAction.javasrc/main/java/org/xoops/support/actions/RefreshXoopsOverviewAction.javasrc/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.javasrc/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.javasrc/main/java/org/xoops/support/completion/XoopsLanguageConstantsCache.javasrc/main/java/org/xoops/support/inspections/CreateMissingTemplateQuickFix.javasrc/main/java/org/xoops/support/inspections/DocumentEditHelper.javasrc/main/java/org/xoops/support/inspections/InsertBeforeOffsetQuickFix.javasrc/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.javasrc/main/java/org/xoops/support/inspections/PhpTextUtil.javasrc/main/java/org/xoops/support/inspections/ReplaceRangeQuickFix.javasrc/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.javasrc/main/java/org/xoops/support/inspections/XoopsIncludeOnceHeaderInspection.javasrc/main/java/org/xoops/support/inspections/XoopsMissingRegisteredTemplateInspection.javasrc/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.javasrc/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.javasrc/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.javasrc/main/java/org/xoops/support/scanner/CoreProfile.javasrc/main/java/org/xoops/support/scanner/XoopsFinding.javasrc/main/java/org/xoops/support/scanner/XoopsModuleInfo.javasrc/main/java/org/xoops/support/scanner/XoopsProjectReport.javasrc/main/java/org/xoops/support/scanner/XoopsProjectScanner.javasrc/main/java/org/xoops/support/settings/XoopsConfigurable.javasrc/main/java/org/xoops/support/settings/XoopsSettingsState.javasrc/main/java/org/xoops/support/ui/XoopsReportHtmlRenderer.javasrc/main/java/org/xoops/support/ui/XoopsToolWindowFactory.javasrc/main/java/org/xoops/support/ui/XoopsToolWindowPanel.javasrc/main/resources/META-INF/plugin.xmlsrc/main/resources/inspectionDescriptions/XoopsDeprecatedDbApi.htmlsrc/main/resources/inspectionDescriptions/XoopsIncludeOnceHeader.htmlsrc/main/resources/inspectionDescriptions/XoopsMissingRegisteredTemplate.htmlsrc/main/resources/inspectionDescriptions/XoopsQueryExec.htmlsrc/main/resources/inspectionDescriptions/XoopsResultSetGuard.htmlsrc/main/resources/inspectionDescriptions/XoopsRootPathGuard.htmlsrc/main/resources/inspectionDescriptions/XoopsSuperglobal.htmlsrc/main/resources/inspectionDescriptions/XoopsWrongSmartyDelimiter.htmlsrc/main/resources/liveTemplates/Xoops.xmltest-fixtures/bad_module_sample.phptest-fixtures/bad_template_sample.tplwhats-new.html
Comment on lines
+54
to
+80
| String msg = "XOOPS: prefer \\Xmf\\Request over $_" + source; | ||
| LocalQuickFix[] fixes; | ||
| if ("REQUEST".equals(source)) { | ||
| // Do not invent a single source — $_REQUEST merges GET/POST/COOKIE. | ||
| fixes = new LocalQuickFix[]{ | ||
| replaceFix("Use Request GET", keyed.start(), keyed.end(), matched, | ||
| "\\Xmf\\Request::getString('" + key + "', '', 'GET')"), | ||
| replaceFix("Use Request POST", keyed.start(), keyed.end(), matched, | ||
| "\\Xmf\\Request::getString('" + key + "', '', 'POST')"), | ||
| replaceFix("Use Request COOKIE", keyed.start(), keyed.end(), matched, | ||
| "\\Xmf\\Request::getString('" + key + "', '', 'COOKIE')"), | ||
| }; | ||
| msg = "XOOPS: prefer \\Xmf\\Request over $_REQUEST " | ||
| + "(choose GET, POST, or COOKIE — $_REQUEST merges sources)"; | ||
| } else { | ||
| String methodSource = switch (source) { | ||
| case "POST" -> "POST"; | ||
| case "COOKIE" -> "COOKIE"; | ||
| default -> "GET"; | ||
| }; | ||
| String replacement = "\\Xmf\\Request::getString('" + key + "', '', '" + methodSource + "')"; | ||
| fixes = new LocalQuickFix[]{ | ||
| replaceFix("Replace with Xmf\\Request::getString()", | ||
| keyed.start(), keyed.end(), matched, replacement) | ||
| }; | ||
| } | ||
| holder.registerProblem(leaf, msg, fixes); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
file="src/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.java"
ast-grep outline "$file" --match '$_' --view expanded 2>/dev/null || true
sed -n '1,180p' "$file"
printf '\n--- request accessor usages ---\n'
rg -n "get(String|Int|Boolean|Array)|Xmf\\\\Request|superglobal|REQUEST|POST|COOKIE|GET" src/main/java
printf '\n--- inspection tests and docs ---\n'
rg -n "XoopsSuperglobalInspection|\\$_(GET|POST|COOKIE|REQUEST)|getString" . -g '*.java' -g '*.md' -g '*.xml' -g '*.json' -g '*.yml' -g '*.yaml' 2>/dev/null | head -300Repository: XOOPS/phpstorm-plugin
Length of output: 10096
🌐 Web query:
XOOPS Xmf Request getString getArray getInt getBool API
💡 Result:
The Xmf\Request class in the XOOPS XMF library provides a static interface for accessing and filtering HTTP request variables (such as $_GET, $_POST, and $_REQUEST) in a secure manner [1][2]. The class methods automatically sanitize input to prevent injection attacks [1][2]. Below is a summary of the requested methods: getInt($name, $default = 0, $hash = 'default') Fetches a request variable and ensures it is returned as an integer [1][2]. The default value is 0 [2]. getString($name, $default = '', $hash = 'default', $mask = 0) Fetches a request variable as a string [1][3]. The optional mask parameter allows for specific filtering [1][2]. getArray($name, $default = [], $hash = 'default') Fetches a request variable as an array [1][2]. If the variable is not found, it returns the provided default array [1][3]. getBool($name, $default = false, $hash = 'default') Fetches a request variable and returns it as a boolean (true/false) [1][3]. The default value is false [1]. Common Parameters: - $name: The name of the variable to retrieve [1][4]. - $default: The value to return if the variable does not exist [1][4]. - $hash: The source of the request data (defaults to 'default', which typically checks _REQUEST) [1][2]. Usage Example: use Xmf\Request; $id = Request::getInt('id', 0); $name = Request::getString('username', 'Guest'); $data = Request::getArray('preferences', []); $isActive = Request::getBool('active', false); The Xmf\Request class is intended to replace the older XoopsRequest class in modern XOOPS development [5][6]. Since it uses PSR-4 autoloading, it can be used directly by importing the namespace [5].
Citations:
- 1: https://api.xoops.org/api2.5.11/apigen/Xmf.Request.html
- 2: https://xoops.gitbook.io/xmf-cookbook/reference/request
- 3: https://api.monxoops.fr/api-xoops2511b2/d0/d1e/class_xmf_1_1_request.html
- 4: https://api.xoops.org/2.5.9/class-Xmf.Request.html
- 5: https://xoops.gitbook.io/xmf-cookbook/basic-ingredients/autoloading
- 6: https://xoops.gitbook.io/xoops-modules-cookbook/best-practices/best-practices/addnamespaces
Remove the unconditional getString() quick fixes.
\Xmf\Request::getString() converts values to strings. A keyed superglobal can contain an array, such as $_POST['ids']; use getArray() when the value is an array. The $_REQUEST fixes also force a source and can change merged-source behavior.
Only offer an automatic replacement when PSI usage analysis proves the value is a string and the source is known. Otherwise, report the inspection without a replacement and let the developer select the appropriate accessor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.java`
around lines 54 - 80, Update the inspection logic around the keyed superglobal
quick-fix construction and holder.registerProblem so automatic replacements are
offered only when PSI analysis proves the value is a string and the source is
known. Remove unconditional getString() fixes, including all forced-source fixes
for $_REQUEST; when usage or source is ambiguous, report the problem with no
replacement and leave accessor selection to the developer. Preserve getArray()
handling for proven array values.
Comment on lines
+60
to
+68
| <localInspection | ||
| language="PHP" | ||
| groupPath="PHP" | ||
| groupName="XOOPS" | ||
| enabledByDefault="true" | ||
| level="WARNING" | ||
| implementationClass="org.xoops.support.inspections.XoopsRootPathGuardInspection" | ||
| displayName="Missing XOOPS_ROOT_PATH direct-access guard" | ||
| shortName="XoopsRootPathGuard"/> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the project enabled setting effective for inspections and completion.
XoopsConfigurable stores enabled, but the registered XoopsRootPathGuardInspection and XoopsLanguageConstantCompletionContributor do not check it. These features continue to run after a user disables XOOPS Support for the project.
Add a shared enabled-state check to every inspection and completion provider that this plugin registers. This must include the root-path inspection and language-constant contributor.
Also applies to: 140-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/META-INF/plugin.xml` around lines 60 - 68, Make the
project-level enabled setting from XoopsConfigurable control all registered
inspections and completion providers. Add or reuse a shared enabled-state check
in XoopsRootPathGuardInspection and XoopsLanguageConstantCompletionContributor,
plus every other registered provider, so they stop running when XOOPS Support is
disabled while preserving their current behavior when enabled.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.java`:
- Around line 88-91: Update InsertRootPathGuardQuickFix to handle files
beginning with a short <? tag before the fallback insertion: detect the leading
short tag and insert GUARD immediately after it, or suppress the quick fix for
such files. Preserve the existing handling for <?php and <?= and avoid
prepending a second opening tag.
In `@src/main/java/org/xoops/support/inspections/PhpTextUtil.java`:
- Around line 51-124: Extend maskCommentsAndStrings to recognize PHP heredoc and
nowdoc starts (`<<<IDENT`, `<<<'IDENT'`, and `<<<"IDENT"`), mask the declaration
and every body character through the matching closing identifier, and preserve
all original offsets. Ensure subsequent inspection regexes cannot match content
inside these regions while leaving unrelated PHP code unchanged.
In `@src/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.java`:
- Around line 37-52: Update the three inspections to preserve executable literal
tokens and source offsets while excluding comments: in
src/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.java:37-52,
change the QUERY_CALL matching flow to recognize quoted query arguments before
masking; in
src/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.java:74-84,
classify literal SQL from the original source so SELECT statements are not
offered an invalid exec() conversion; and in
src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java:107-111,
detect defined('XOOPS_ROOT_PATH') before masking so valid guards do not trigger
duplicate quick fixes. Use PSI or a scanner that excludes comments while
retaining literal tokens and offsets.
In
`@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java`:
- Around line 49-50: Update the fetch-call matching logic in
XoopsResultSetGuardInspection to scan PhpTextUtil.maskCommentsAndStrings(text)
rather than the raw text, preventing matches inside comments or strings.
Preserve the masked text’s offsets, and use those offsets to retrieve any
required original source text from text while retaining the existing
isFetchAlreadyGuarded check and guard insertion behavior.
In
`@src/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java`:
- Around line 88-92: Update the literal-block handling in
XoopsWrongSmartyDelimiterInspection to search for the correct Smarty closing
delimiter `<{/literal}>` wherever the closing marker is located and its length
is calculated, preserving the existing masking behavior for valid literal
blocks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 14678723-ae19-433f-93f6-69d399572629
📒 Files selected for processing (20)
GITHUB_SETUP.mdREADME.mdbuild.gradle.ktssrc/main/java/org/xoops/support/XoopsSupportPlugin.javasrc/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.javasrc/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.javasrc/main/java/org/xoops/support/completion/XoopsLanguageConstantsCache.javasrc/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.javasrc/main/java/org/xoops/support/inspections/PhpTextUtil.javasrc/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.javasrc/main/java/org/xoops/support/inspections/XoopsIncludeOnceHeaderInspection.javasrc/main/java/org/xoops/support/inspections/XoopsMissingRegisteredTemplateInspection.javasrc/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.javasrc/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.javasrc/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.javasrc/main/java/org/xoops/support/scanner/XoopsProjectScanner.javasrc/main/resources/inspectionDescriptions/XoopsResultSetGuard.htmltest-fixtures/bad_module_sample.php
🚧 Files skipped from review as they are similar to previous changes (12)
- src/main/java/org/xoops/support/inspections/XoopsMissingRegisteredTemplateInspection.java
- src/main/resources/inspectionDescriptions/XoopsResultSetGuard.html
- GITHUB_SETUP.md
- src/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.java
- src/main/java/org/xoops/support/completion/XoopsLanguageConstantCompletionContributor.java
- build.gradle.kts
- src/main/java/org/xoops/support/inspections/XoopsIncludeOnceHeaderInspection.java
- src/main/java/org/xoops/support/completion/XoopsLanguageConstantsCache.java
- src/main/java/org/xoops/support/inspections/XoopsSuperglobalInspection.java
- README.md
- src/main/java/org/xoops/support/scanner/XoopsProjectScanner.java
- test-fixtures/bad_module_sample.php
Address CodeRabbit and Greptile review findings on XOOPS Support:
- Result-set guard: match outside comments/strings; only suppress dominating
isResultSet checks; compound positives like (!$error && isResultSet($r)) work;
quick fix always throws (never emit continue outside loops)
- Root-path guard: first executable after <?php must be a terminating guard;
QF handles <?php, short <?, and <?= without double open tags
- query/queryF → exec only for single-arg calls; SELECT never offered exec;
classify SQL from original string literals (comments masked separately)
- PhpTextUtil: mask comments/strings/heredoc-nowdoc with stable offsets;
firstStringArgContent helper for call-site analysis
- Superglobal: keyed GET/POST/COOKIE getString fixes; bare/REQUEST warn-only
- Language-constant cache: CachedValue + PsiModificationTracker; VFS invalidation
- Settings enabled gate on inspections and completion
- Smarty: mask {* *} and <{literal}>…<{/literal}> (correct closer)
- Composer profile: bind xoops package name to its version constraint
- CI: pin third-party actions to commit SHAs; persist-credentials false
- Docs/fixtures: GITHUB_SETUP init vs clone; README QF scope; rebrand samples
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java (3)
221-225: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe static analysis ReDoS hint is a false positive.
resultVaris captured by group 3 ofFETCH, which allows only\$[A-Za-z_][\w]*. The value also passes throughPattern.quote. The compiled expression has no nested quantifiers over the injected text. No action is needed for the security hint.Consider caching the compiled
PatternperresultVarto avoid repeated compilation in the match loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java` around lines 221 - 225, Retain the existing safely quoted regex in conditionMentionsIsResultSet; no ReDoS mitigation is needed. Optimize repeated matching by caching the compiled Pattern per resultVar and reusing it instead of compiling on every invocation.Source: Linters/SAST tools
92-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead branch and the duplicate early-exit check.
The three blocks test the same state.
- Line 94 uses
before.substring(Math.min(last.ifEnd, before.length())). Iflast.ifEnd >= before.length(), the substring is empty, soisOnlyWhitespacereturns true and the first block runs.- The
else ifat Line 100 therefore runs only whenlast.ifEnd < before.length(). Its own conditionlast.ifEnd >= before.length()is then always false. This branch is unreachable.- The block at Line 107 repeats the first block for
last.ifEnd <= before.length().Keep one check.
♻️ Proposed simplification
IfCond last = ifs.get(ifs.size() - 1); - if (isOnlyWhitespace(before.substring(Math.min(last.ifEnd, before.length())))) { - // if construct consumed through end of before (still “attached” to the fetch) - if (conditionNegatesIsResultSet(last.condition, resultVar) - && bodyHasEarlyExit(before, last)) { - return true; - } - } else if (last.ifEnd >= before.length() - && conditionNegatesIsResultSet(last.condition, resultVar) - && bodyHasEarlyExit(before, last)) { - return true; - } - - // Also: early-exit if ends with only whitespace after ifEnd - if (last.ifEnd <= before.length() - && isOnlyWhitespace(before.substring(last.ifEnd)) - && conditionNegatesIsResultSet(last.condition, resultVar) - && bodyHasEarlyExit(before, last)) { - return true; - } + // The if construct is consumed through the end of `before`, so it stays attached to the fetch. + if (isOnlyWhitespace(before.substring(Math.min(last.ifEnd, before.length()))) + && conditionNegatesIsResultSet(last.condition, resultVar) + && bodyHasEarlyExit(before, last)) { + return true; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java` around lines 92 - 112, In the early-exit handling for the final IfCond last, remove the unreachable else-if branch and the duplicate whitespace check. Keep one condition that verifies the remaining text after last.ifEnd is only whitespace, last.condition negates resultVar, and bodyHasEarlyExit(before, last) is true, preserving the existing return behavior.
52-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winScan the if-conditions once per file.
isFetchAlreadyGuardedcallsfindIfConditions(before)for every fetch match. Each call copies the prefix (Line 56) and rescans it from offset 0. For a file withkfetch calls andncharacters this costs O(k·n).bodyHasEarlyExitalso compiles the same early-exitPatternon every call.Compute the
IfCondlist once per file, then select the relevant entries by offset. Hoist the early-exit pattern into astatic finalfield.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java` around lines 52 - 59, Update the file-level fetch scanning around the FETCH matcher to compute findIfConditions once and reuse the resulting IfCond list, selecting only conditions relevant to each fetch by offset instead of passing a copied prefix to isFetchAlreadyGuarded. Hoist the early-exit regex used by bodyHasEarlyExit into a static final Pattern so it is compiled once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.java`:
- Around line 24-25: Update the OPEN_SHORT pattern in
InsertRootPathGuardQuickFix to exclude xml alongside php and = in its negative
lookahead, so XML prologs are not treated as short PHP tags. Apply the same
pattern change to XoopsRootPathGuardInspection if it defines an equivalent
short-tag pattern.
In `@src/main/java/org/xoops/support/inspections/PhpTextUtil.java`:
- Around line 111-131: Update the closer detection in the heredoc-masking loop
around the closer boolean so it recognizes the identifier at the start of the
trimmed line, while accepting any following token except letters, digits, or
underscores. Preserve support for bare identifiers and identifiers followed by
semicolons, and ensure valid closers such as “SQL)” stop masking so downstream
maskCommentsAndStrings consumers continue processing.
In
`@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java`:
- Around line 122-151: Update the IF_KEYWORD pattern used by findIfConditions to
match both standalone if and elseif forms, including the existing else if form,
while preserving the current parenthesis and body parsing behavior.
---
Nitpick comments:
In
`@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java`:
- Around line 221-225: Retain the existing safely quoted regex in
conditionMentionsIsResultSet; no ReDoS mitigation is needed. Optimize repeated
matching by caching the compiled Pattern per resultVar and reusing it instead of
compiling on every invocation.
- Around line 92-112: In the early-exit handling for the final IfCond last,
remove the unreachable else-if branch and the duplicate whitespace check. Keep
one condition that verifies the remaining text after last.ifEnd is only
whitespace, last.condition negates resultVar, and bodyHasEarlyExit(before, last)
is true, preserving the existing return behavior.
- Around line 52-59: Update the file-level fetch scanning around the FETCH
matcher to compute findIfConditions once and reuse the resulting IfCond list,
selecting only conditions relevant to each fetch by offset instead of passing a
copied prefix to isFetchAlreadyGuarded. Hoist the early-exit regex used by
bodyHasEarlyExit into a static final Pattern so it is compiled once.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7fe73a7-b773-463b-ba14-2e0af603cec3
📒 Files selected for processing (7)
src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.javasrc/main/java/org/xoops/support/inspections/PhpTextUtil.javasrc/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.javasrc/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.javasrc/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java
- src/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.java
- src/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.java
- src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java
Address remaining CodeRabbit/Greptile findings on XOOPS Support inspections: - Result-set guard: parse if/elseif with balanced parentheses; support brace-less positive guards (if (isResultSet($r)) fetch…); compound conditions with nested calls (count($errors) === 0 && isResultSet($r)); parse if conditions once per file; static early-exit / isResultSet patterns - Heredoc/nowdoc closers: accept IDENT followed by non-word tokens (e.g. SQL)) - Short PHP open tag: exclude xml from <? short-tag lookahead (avoid XML prolog) - Keep throw-only failure quick fix (never emit continue outside loops)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java (1)
84-99: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse a scope-safe, statement-level insertion point for the guard.
lineStart(text, fetchOffset)can validate$resultbefore a same-line assignment or outside a single-line conditional. Use PSI to locate the enclosing statement. Offer the quick fix only when the insertion preserves control flow; otherwise register the problem without a quick fix.The positive-guard analysis also accepts conditions such as
isResultSet($result) || $fallback, although$fallbackcan allowfetch*when$resultis invalid. Require a condition that guaranteesisResultSet($result)before suppressing the warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java` around lines 84 - 99, Update the guard quick-fix logic around InsertBeforeOffsetQuickFix to locate the enclosing PSI statement and insert only at a scope-safe statement boundary, preserving control flow; when no safe insertion point exists, register the inspection problem without a quick fix. Tighten the positive-guard analysis so conditions suppressing the warning must guarantee isResultSet($result), rejecting OR branches such as isResultSet($result) || $fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java`:
- Around line 128-139: Make the early-exit and positive-guard suppression logic
path-safe: do not rely on bodyHasEarlyExit or conditionMentionsIsResultSet alone
when exits are conditional/nested or the check appears on an unsafe || path.
Update the relevant guard analysis methods, including
isInsidePositiveIsResultSetGuard, to use PHP PSI control-flow analysis or
conservatively accept only guard forms proving every path to fetch* satisfies
the result-set condition; preserve findings for the shown unsafe patterns.
---
Outside diff comments:
In
`@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java`:
- Around line 84-99: Update the guard quick-fix logic around
InsertBeforeOffsetQuickFix to locate the enclosing PSI statement and insert only
at a scope-safe statement boundary, preserving control flow; when no safe
insertion point exists, register the inspection problem without a quick fix.
Tighten the positive-guard analysis so conditions suppressing the warning must
guarantee isResultSet($result), rejecting OR branches such as
isResultSet($result) || $fallback.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ba190ee-5ae4-436d-b3d6-f6e20cc552e4
📒 Files selected for processing (7)
src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.javasrc/main/java/org/xoops/support/inspections/PhpTextUtil.javasrc/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.javasrc/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.javasrc/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.javasrc/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/java/org/xoops/support/inspections/PhpTextUtil.java
- src/main/java/org/xoops/support/inspections/XoopsWrongSmartyDelimiterInspection.java
- src/main/java/org/xoops/support/inspections/InsertRootPathGuardQuickFix.java
- src/main/java/org/xoops/support/inspections/XoopsQueryExecInspection.java
- src/main/java/org/xoops/support/inspections/XoopsDeprecatedDbApiInspection.java
- src/main/java/org/xoops/support/inspections/XoopsRootPathGuardInspection.java
Tighten XoopsResultSetGuardInspection so findings are only suppressed when every path to fetch* is proven safe (without full PHP CFG): - Positive if: require isResultSet($var), reject top-level ||/or (e.g. isResultSet($r) || $fallback still warns) - Early-exit if: require negation of isResultSet (incl. parenthesized !($db->isResultSet($r))), reject top-level &&/and (e.g. !isResultSet($r) && $strict still warns), body must be a single exit - Keep throw-only quick fix at PSI Statement boundaries
Parent the language-constant VFS AsyncFileListener on the project service Disposable instead of the Project, so the listener is removed on plugin unload and no longer pins the plugin classloader. Also dispose tool-window listeners with content, expire deferred invokeLater work on dispose/unload, and clear the project cache key in dispose(). Verified: DynamicPlugins logs "loaded without restart".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/xoops/support/ui/XoopsToolWindowPanel.java (1)
91-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent an older scan from overwriting the latest refresh.
RefreshXoopsOverviewActioncallsrefresh()directly. It bypasses the disabled button. Two scans can run concurrently, and the older scan can apply its report after a newer scan completes.Assign each refresh a monotonic request ID. Apply a report or error only when its request ID is still current.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xoops/support/ui/XoopsToolWindowPanel.java` around lines 91 - 115, Update the refresh flow around the background Task in XoopsToolWindowPanel so each refresh receives a monotonically increasing request ID, including refreshes triggered directly by RefreshXoopsOverviewAction. Capture that ID in both success and error callbacks, and call applyReport or applyError only when it matches the latest request ID, preventing older scans from overwriting newer results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.java`:
- Around line 344-366: Update isSafePositiveGuardCondition and
isSafeEarlyExitCondition so boolean operators inside parentheses are analyzed
rather than ignored by hasTopLevelBoolOp. Recursively inspect the condition, or
conservatively reject any nested || in positive guards and nested && in
early-exit guards, preserving safe conditions while preventing these
parenthesized bypasses.
- Around line 327-338: Update isWordAt so both leftOk and rightOk use
Character.isJavaIdentifierPart for boundary checks instead of
Character.isLetterOrDigit, treating underscores and other Java identifier
characters as part of identifiers while preserving the existing case-insensitive
word matching.
In `@src/main/java/org/xoops/support/ui/XoopsToolWindowPanel.java`:
- Around line 102-113: Update the scan failure handling around applyError to
avoid catching Throwable: catch expected scan exceptions, rethrow
ProcessCanceledException before handling failures, and preserve the existing
escaped HTML error reporting for non-cancellation exceptions.
---
Outside diff comments:
In `@src/main/java/org/xoops/support/ui/XoopsToolWindowPanel.java`:
- Around line 91-115: Update the refresh flow around the background Task in
XoopsToolWindowPanel so each refresh receives a monotonically increasing request
ID, including refreshes triggered directly by RefreshXoopsOverviewAction.
Capture that ID in both success and error callbacks, and call applyReport or
applyError only when it matches the latest request ID, preventing older scans
from overwriting newer results.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76ca5547-d09b-4209-9327-dba0137670dc
📒 Files selected for processing (6)
src/main/java/org/xoops/support/XoopsStartupActivity.javasrc/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.javasrc/main/java/org/xoops/support/completion/XoopsLanguageConstantsCache.javasrc/main/java/org/xoops/support/inspections/XoopsResultSetGuardInspection.javasrc/main/java/org/xoops/support/ui/XoopsToolWindowFactory.javasrc/main/java/org/xoops/support/ui/XoopsToolWindowPanel.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/xoops/support/ui/XoopsToolWindowFactory.java
- src/main/java/org/xoops/support/XoopsStartupActivity.java
- src/main/java/org/xoops/support/actions/ShowXoopsProjectInfoAction.java
- src/main/java/org/xoops/support/completion/XoopsLanguageConstantsCache.java
Reject parenthesized ||/&& in isResultSet guard conditions, treat identifier boundaries with isJavaIdentifierPart, and rethrow ProcessCanceledException from overview scans. Sequence refresh callbacks so an older scan cannot overwrite a newer result.
Comment on lines
+300
to
+320
| private static boolean hasBoolOpAnywhere(@NotNull String cond, boolean orOp) { | ||
| for (int i = 0; i < cond.length(); i++) { | ||
| char c = cond.charAt(i); | ||
| if (orOp) { | ||
| if (c == '|' && i + 1 < cond.length() && cond.charAt(i + 1) == '|') { | ||
| return true; | ||
| } | ||
| if (isWordAt(cond, i, "or")) { | ||
| return true; | ||
| } | ||
| } else { | ||
| if (c == '&' && i + 1 < cond.length() && cond.charAt(i + 1) == '&') { | ||
| return true; | ||
| } | ||
| if (isWordAt(cond, i, "and")) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
When a fetch is inside a condition such as if ($db->isResultSet($result) xor $fallback), the boolean-operator scan does not recognize PHP's xor, so the condition is accepted as a positive guard even though its body can run while the result-set check is false, suppressing the unsafe-fetch warning.
Suggested change
| private static boolean hasBoolOpAnywhere(@NotNull String cond, boolean orOp) { | |
| for (int i = 0; i < cond.length(); i++) { | |
| char c = cond.charAt(i); | |
| if (orOp) { | |
| if (c == '|' && i + 1 < cond.length() && cond.charAt(i + 1) == '|') { | |
| return true; | |
| } | |
| if (isWordAt(cond, i, "or")) { | |
| return true; | |
| } | |
| } else { | |
| if (c == '&' && i + 1 < cond.length() && cond.charAt(i + 1) == '&') { | |
| return true; | |
| } | |
| if (isWordAt(cond, i, "and")) { | |
| return true; | |
| } | |
| } | |
| } | |
| return false; | |
| } | |
| private static boolean hasBoolOpAnywhere(@NotNull String cond, boolean orOp) { | |
| for (int i = 0; i < cond.length(); i++) { | |
| char c = cond.charAt(i); | |
| if (isWordAt(cond, i, "xor")) { | |
| return true; | |
| } | |
| if (orOp) { | |
| if (c == '|' && i + 1 < cond.length() && cond.charAt(i + 1) == '|') { | |
| return true; | |
| } | |
| if (isWordAt(cond, i, "or")) { | |
| return true; | |
| } | |
| } else { | |
| if (c == '&' && i + 1 < cond.length() && cond.charAt(i + 1) == '&') { | |
| return true; | |
| } | |
| if (isWordAt(cond, i, "and")) { | |
| return true; | |
| } | |
| } | |
| } | |
| return false; | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
XOOPS Support for PhpStorm: inspections + Alt+Enter fixes, language-constant completion, live templates, module scaffold, and a project scanner. Built for real XOOPS 2.5 / 2.7 / 4.0 work. Install from disk and try Tools → XOOPS Support.
Summary by Sourcery
Introduce the initial 1.0.0 Alpha 1 release of the XOOPS Support PhpStorm plugin, providing XOOPS-aware tooling (inspections, completion, templates, scanner, and module scaffolding) along with build, CI, and documentation setup.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Chores