From 8d0db1e78b83aa2ebd3deb1f87a9745f4dc2a167 Mon Sep 17 00:00:00 2001 From: Daniel Espendiller Date: Tue, 28 Mar 2023 17:38:13 +0200 Subject: [PATCH] support detection for duplicate routes in same file for xml, php attributes and docblock --- .../XmlDuplicateParameterKeyInspection.java | 2 +- .../XmlDuplicateServiceKeyInspection.java | 6 +- .../inspection/EventMethodCallInspection.java | 2 +- .../DuplicateLocalRouteInspection.java | 91 +++++++++++++++++- .../symfony2plugin/util/PhpElementsUtil.java | 21 ++++- src/main/resources/META-INF/plugin.xml | 4 +- .../DuplicateLocalRouteInspection.html | 6 ++ .../DuplicateLocalRouteInspectionTest.java | 94 ++++++++++++++++++- .../DuplicateLocalRouteInspection.php | 19 ++++ 9 files changed, 231 insertions(+), 14 deletions(-) create mode 100644 src/main/resources/inspectionDescriptions/DuplicateLocalRouteInspection.html create mode 100644 src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/fixtures/DuplicateLocalRouteInspection.php diff --git a/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateParameterKeyInspection.java b/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateParameterKeyInspection.java index 566deda25..2d2d97b8a 100644 --- a/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateParameterKeyInspection.java +++ b/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateParameterKeyInspection.java @@ -23,7 +23,7 @@ public PsiElementVisitor buildVisitor(final @NotNull ProblemsHolder holder, bool @Override public void visitElement(@NotNull PsiElement element) { if (element instanceof XmlAttributeValue xmlAttributeValue) { - visitRoot(xmlAttributeValue, holder, "parameters", "parameter", "key"); + visitRoot(xmlAttributeValue, holder, "parameters", "parameter", "key", "Symfony: Duplicate Key"); } super.visitElement(element); diff --git a/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateServiceKeyInspection.java b/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateServiceKeyInspection.java index 7c33379c1..f5f6c617f 100644 --- a/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateServiceKeyInspection.java +++ b/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateServiceKeyInspection.java @@ -27,7 +27,7 @@ public PsiElementVisitor buildVisitor(final @NotNull ProblemsHolder holder, bool @Override public void visitElement(@NotNull PsiElement element) { if (element instanceof XmlAttributeValue xmlAttributeValue) { - visitRoot(xmlAttributeValue, holder, "services", "service", "id"); + visitRoot(xmlAttributeValue, holder, "services", "service", "id", "Symfony: Duplicate Key"); } super.visitElement(element); @@ -35,7 +35,7 @@ public void visitElement(@NotNull PsiElement element) { }; } - protected void visitRoot(@NotNull XmlAttributeValue xmlAttributeValue, @NotNull ProblemsHolder holder, String root, String child, String tagName) { + public static void visitRoot(@NotNull XmlAttributeValue xmlAttributeValue, @NotNull ProblemsHolder holder, @NotNull String root, @NotNull String child, @NotNull String tagName, @NotNull String message) { String value = null; if (xmlAttributeValue.getParent() instanceof XmlAttribute xmlAttribute && tagName.equals(xmlAttribute.getName())) { @@ -55,7 +55,7 @@ protected void visitRoot(@NotNull XmlAttributeValue xmlAttributeValue, @NotNull } if (found == 2) { - holder.registerProblem(xmlAttributeValue, "Symfony: Duplicate Key", ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + holder.registerProblem(xmlAttributeValue, message, ProblemHighlightType.GENERIC_ERROR_OR_WARNING); break; } } diff --git a/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/yaml/inspection/EventMethodCallInspection.java b/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/yaml/inspection/EventMethodCallInspection.java index ad7985c0c..595bc2b6b 100644 --- a/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/yaml/inspection/EventMethodCallInspection.java +++ b/src/main/java/fr/adrienbrault/idea/symfony2plugin/config/yaml/inspection/EventMethodCallInspection.java @@ -238,7 +238,7 @@ private static void visitPhpElement(@NotNull StringLiteralExpression element, @N } } - if (parent instanceof ParameterList parameterList && PhpElementsUtil.isAttributeNamedArgumentString(element, "method", "\\Symfony\\Component\\EventDispatcher\\Attribute\\AsEventListener")) { + if (parent instanceof ParameterList parameterList && PhpElementsUtil.isAttributeNamedArgumentString(element, "\\Symfony\\Component\\EventDispatcher\\Attribute\\AsEventListener", "method")) { PhpAttribute parentOfType = PsiTreeUtil.getParentOfType(parameterList, PhpAttribute.class); if (parentOfType.getOwner() instanceof PhpClass phpClass) { String contents = element.getContents(); diff --git a/src/main/java/fr/adrienbrault/idea/symfony2plugin/routing/inspection/DuplicateLocalRouteInspection.java b/src/main/java/fr/adrienbrault/idea/symfony2plugin/routing/inspection/DuplicateLocalRouteInspection.java index 44ae9f59c..27509ca66 100644 --- a/src/main/java/fr/adrienbrault/idea/symfony2plugin/routing/inspection/DuplicateLocalRouteInspection.java +++ b/src/main/java/fr/adrienbrault/idea/symfony2plugin/routing/inspection/DuplicateLocalRouteInspection.java @@ -5,9 +5,27 @@ import com.intellij.codeInspection.ProblemsHolder; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiElementVisitor; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.xml.XmlAttributeValue; +import com.jetbrains.php.lang.PhpLanguage; +import com.jetbrains.php.lang.documentation.phpdoc.psi.PhpDocComment; +import com.jetbrains.php.lang.documentation.phpdoc.psi.tags.PhpDocTag; +import com.jetbrains.php.lang.psi.elements.Method; +import com.jetbrains.php.lang.psi.elements.PhpAttribute; +import com.jetbrains.php.lang.psi.elements.PhpClass; +import com.jetbrains.php.lang.psi.elements.StringLiteralExpression; +import de.espend.idea.php.annotation.dict.PhpDocCommentAnnotation; +import de.espend.idea.php.annotation.dict.PhpDocTagAnnotation; +import de.espend.idea.php.annotation.pattern.AnnotationPattern; +import de.espend.idea.php.annotation.util.AnnotationUtil; import fr.adrienbrault.idea.symfony2plugin.Symfony2ProjectComponent; +import fr.adrienbrault.idea.symfony2plugin.config.xml.inspection.XmlDuplicateServiceKeyInspection; +import fr.adrienbrault.idea.symfony2plugin.routing.RouteHelper; +import fr.adrienbrault.idea.symfony2plugin.util.PhpElementsUtil; import fr.adrienbrault.idea.symfony2plugin.util.yaml.YamlHelper; +import org.apache.commons.lang.StringUtils; import org.jetbrains.annotations.NotNull; +import org.jetbrains.yaml.YAMLLanguage; import org.jetbrains.yaml.psi.YAMLDocument; import org.jetbrains.yaml.psi.YAMLKeyValue; import org.jetbrains.yaml.psi.YAMLMapping; @@ -16,6 +34,7 @@ * @author Daniel Espendiller */ public class DuplicateLocalRouteInspection extends LocalInspectionTool { + private static final String MESSAGE = "Symfony: Duplicate route name"; @NotNull @Override @@ -36,7 +55,19 @@ public MyPsiElementVisitor(ProblemsHolder holder) { @Override public void visitElement(@NotNull PsiElement element) { - if (element instanceof YAMLKeyValue yamlKeyValue && YamlHelper.isRoutingFile(yamlKeyValue.getContainingFile()) && yamlKeyValue.getParent() instanceof YAMLMapping yamlMapping && yamlMapping.getParent() instanceof YAMLDocument) { + if (element instanceof YAMLKeyValue yamlKeyValue && element.getLanguage() == YAMLLanguage.INSTANCE) { + visitYaml(yamlKeyValue); + } else if(element instanceof StringLiteralExpression && element.getLanguage() == PhpLanguage.INSTANCE) { + visitPhp((StringLiteralExpression) element); + } else if(element instanceof XmlAttributeValue xmlAttributeValue) { + XmlDuplicateServiceKeyInspection.visitRoot(xmlAttributeValue, holder, "routes", "route", "id", MESSAGE); + } + + super.visitElement(element); + } + + private void visitYaml(@NotNull YAMLKeyValue yamlKeyValue) { + if (YamlHelper.isRoutingFile(yamlKeyValue.getContainingFile()) && yamlKeyValue.getParent() instanceof YAMLMapping yamlMapping && yamlMapping.getParent() instanceof YAMLDocument) { String keyText1 = null; int found = 0; @@ -55,14 +86,68 @@ public void visitElement(@NotNull PsiElement element) { if (found == 2) { final PsiElement keyElement = yamlKeyValue.getKey(); assert keyElement != null; - holder.registerProblem(keyElement, "Symfony: Duplicate key", ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + holder.registerProblem(keyElement, "Symfony: Duplicate route name", ProblemHighlightType.GENERIC_ERROR_OR_WARNING); break; } } } + } - super.visitElement(element); + private void visitPhp(@NotNull StringLiteralExpression element) { + if (PhpElementsUtil.isAttributeNamedArgumentString(element, "\\Symfony\\Component\\Routing\\Annotation\\Route", "name")) { + PhpAttribute parentOfType = PsiTreeUtil.getParentOfType(element, PhpAttribute.class); + if (parentOfType.getOwner() instanceof Method method && method.getAccess().isPublic() && method.getContainingClass() != null) { + int found = 0; + String contents = element.getContents(); + + for (Method ownMethod : method.getContainingClass().getOwnMethods()) { + for (PhpAttribute attribute : ownMethod.getAttributes("\\Symfony\\Component\\Routing\\Annotation\\Route")) { + String name = PhpElementsUtil.getAttributeArgumentStringByName(attribute, "name"); + if (contents.equals(name)) { + found++; + } + + if (found == 2) { + holder.registerProblem(element, MESSAGE, ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + return; + } + } + } + } + } + + if (AnnotationPattern.getPropertyIdentifierValue("name").accepts(element)) { + PhpDocTag phpDocTag = PsiTreeUtil.getParentOfType(element, PhpDocTag.class); + if (phpDocTag != null) { + PhpClass phpClass = AnnotationUtil.getAnnotationReference(phpDocTag); + if (phpClass != null && RouteHelper.ROUTE_CLASSES.contains(StringUtils.stripStart(phpClass.getFQN(), "\\"))) { + PhpDocComment phpDocComment = PsiTreeUtil.getParentOfType(element, PhpDocComment.class); + if (phpDocComment.getNextPsiSibling() instanceof Method method && method.getAccess().isPublic() && method.getContainingClass() != null) { + int found = 0; + String contents = element.getContents(); + + for (Method ownMethod : method.getContainingClass().getOwnMethods()) { + PhpDocCommentAnnotation phpClassContainer = AnnotationUtil.getPhpDocCommentAnnotationContainer(ownMethod.getDocComment()); + if(phpClassContainer != null) { + PhpDocTagAnnotation firstPhpDocBlock = phpClassContainer.getFirstPhpDocBlock(RouteHelper.ROUTE_CLASSES.toArray(String[]::new)); + if(firstPhpDocBlock != null) { + String name = firstPhpDocBlock.getPropertyValue("name"); + if (contents.equals(name)) { + found++; + } + + if (found == 2) { + holder.registerProblem(element, MESSAGE, ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + return; + } + } + } + } + } + } + } + } } } } diff --git a/src/main/java/fr/adrienbrault/idea/symfony2plugin/util/PhpElementsUtil.java b/src/main/java/fr/adrienbrault/idea/symfony2plugin/util/PhpElementsUtil.java index 820296b69..928cf1f78 100644 --- a/src/main/java/fr/adrienbrault/idea/symfony2plugin/util/PhpElementsUtil.java +++ b/src/main/java/fr/adrienbrault/idea/symfony2plugin/util/PhpElementsUtil.java @@ -35,6 +35,7 @@ import com.jetbrains.php.lang.psi.elements.impl.PhpDefineImpl; import com.jetbrains.php.lang.psi.resolve.types.PhpType; import com.jetbrains.php.lang.psi.stubs.indexes.expectedArguments.PhpExpectedFunctionArgument; +import com.jetbrains.php.lang.psi.stubs.indexes.expectedArguments.PhpExpectedFunctionClassConstantArgument; import com.jetbrains.php.lang.psi.stubs.indexes.expectedArguments.PhpExpectedFunctionScalarArgument; import com.jetbrains.php.phpunit.PhpUnitUtil; import com.jetbrains.php.refactoring.PhpAliasImporter; @@ -1700,7 +1701,7 @@ public static String insertUseIfNecessary(@NotNull PsiElement phpClass, @NotNull /** * #[AsEventListener(method:'onBarEvent')] */ - public static boolean isAttributeNamedArgumentString(@NotNull StringLiteralExpression element, @NotNull String namedArgument, @NotNull String fqn) { + public static boolean isAttributeNamedArgumentString(@NotNull StringLiteralExpression element, @NotNull String fqn, @NotNull String namedArgument) { PsiElement colon = PsiTreeUtil.prevCodeLeaf(element); if (colon == null || colon.getNode().getElementType() != PhpTokenTypes.opCOLON) { return false; @@ -1907,6 +1908,24 @@ public static PhpExpectedFunctionArgument findAttributeArgumentByName(@NotNull S return null; } + public static String getAttributeArgumentStringByName(@NotNull PhpAttribute phpAttribute, @NotNull String attributeName) { + for (PhpAttribute.PhpAttributeArgument argument : phpAttribute.getArguments()) { + if (!attributeName.equals(argument.getName())) { + continue; + } + + if (argument.getArgument() instanceof PhpExpectedFunctionClassConstantArgument phpExpectedFunctionClassConstantArgument) { + return phpExpectedFunctionClassConstantArgument.getClassFqn(); + } + + if (argument.getArgument() instanceof PhpExpectedFunctionScalarArgument phpExpectedFunctionScalarArgument) { + return PsiElementUtils.trimQuote(phpExpectedFunctionScalarArgument.getNormalizedValue()); + } + } + + return null; + } + @Nullable public static String findAttributeArgumentByNameAsString(@NotNull String attributeName, @NotNull PhpAttribute phpAttribute) { PhpExpectedFunctionArgument attributeArgumentByName = findAttributeArgumentByName(attributeName, phpAttribute); diff --git a/src/main/resources/META-INF/plugin.xml b/src/main/resources/META-INF/plugin.xml index 0b01f35ce..49ee8aaad 100644 --- a/src/main/resources/META-INF/plugin.xml +++ b/src/main/resources/META-INF/plugin.xml @@ -284,10 +284,10 @@ language="PHP" implementationClass="fr.adrienbrault.idea.symfony2plugin.routing.inspection.PhpRouteMissingInspection"/> - + +Duplicate routes are defined in same scope where only one overwrites the other + + + \ No newline at end of file diff --git a/src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/DuplicateLocalRouteInspectionTest.java b/src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/DuplicateLocalRouteInspectionTest.java index fdf0ad17f..a7393af4e 100644 --- a/src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/DuplicateLocalRouteInspectionTest.java +++ b/src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/DuplicateLocalRouteInspectionTest.java @@ -8,6 +8,15 @@ * @see fr.adrienbrault.idea.symfony2plugin.routing.inspection.DuplicateLocalRouteInspection */ public class DuplicateLocalRouteInspectionTest extends SymfonyLightCodeInsightFixtureTestCase { + public void setUp() throws Exception { + super.setUp(); + + myFixture.copyFileToProject("DuplicateLocalRouteInspection.php"); + } + + protected String getTestDataPath() { + return "src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/fixtures"; + } public void testDuplicateRouteKeyProvidesWarning() { assertLocalInspectionContains("routing.yml", "" + @@ -15,7 +24,7 @@ public void testDuplicateRouteKeyProvidesWarning() { " car: foo\n" + "foo:\n" + " car: foo\n", - "Symfony: Duplicate key" + "Symfony: Duplicate route name" ); assertLocalInspectionContains("routing.yml", "" + @@ -23,7 +32,7 @@ public void testDuplicateRouteKeyProvidesWarning() { " car: foo\n" + "foo:\n" + " car: foo\n", - "Symfony: Duplicate key" + "Symfony: Duplicate route name" ); assertLocalInspectionNotContains("routing.yml", "" + @@ -33,7 +42,86 @@ public void testDuplicateRouteKeyProvidesWarning() { " car: foo\n" + "foo:\n" + " car: foo\n", - "Symfony: Duplicate key" + "Symfony: Duplicate route name" + ); + } + + public void testDuplicateRoutingKeysForPhpAttribute() { + assertLocalInspectionContains("controller.php", "_index')]\n" + + " #[Route(name: 'foobar_index')]\n" + + " #[Route(name: 'foobar_index_1')]\n" + + " public function index() {}\n" + + "}\n", + "Symfony: Duplicate route name" + ); + + assertLocalInspectionContains("controller.php", "_index')]\n" + + " public function index() {}\n" + + "\n" + + " #[Route(name: 'foobar_index')]\n" + + " public function foo() {}\n" + + "}\n", + "Symfony: Duplicate route name" + ); + + assertLocalInspectionNotContains("controller.php", "_index')]\n" + + " public function index() {}\n" + + "\n" + + " #[Route(name: 'foobar_index_1')]\n" + + " public function foo() {}\n" + + "}\n", + "Symfony: Duplicate route name" + ); + } + + public void testDuplicateRoutingKeysForPhpDocBlocks() { + assertLocalInspectionContains("controller.php", "st\")\n" + + " */\n" + + " public function index2()\n" + + " {\n" + + " }\n" + + "}\n", + "Symfony: Duplicate route name" + ); + } + + public void testDuplicateRoutingKeysForXml() { + assertLocalInspectionContains("routing.xml", "" + + "\n" + + " \n" + + " _list\" path=\"/blog1\"/>\n" + + "", + "Symfony: Duplicate route name" ); } } diff --git a/src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/fixtures/DuplicateLocalRouteInspection.php b/src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/fixtures/DuplicateLocalRouteInspection.php new file mode 100644 index 000000000..438518046 --- /dev/null +++ b/src/test/java/fr/adrienbrault/idea/symfony2plugin/tests/routing/inspection/fixtures/DuplicateLocalRouteInspection.php @@ -0,0 +1,19 @@ + + * @author Alexander M. Turek + */ + #[\Attribute(\Attribute::IS_REPEATABLE | \Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD)] + class Route + { + } +}