Skip to content

support detection for duplicate routes in same file for xml, php attributes and docblock #2112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 28, 2023
Merged

support detection for duplicate routes in same file for xml, php attributes and docblock #2112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...nbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateParameterKeyInspection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions ...ienbrault/idea/symfony2plugin/config/xml/inspection/XmlDuplicateServiceKeyInspection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ 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);
}
};
}

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())) {
Expand All @@ -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;
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...fr/adrienbrault/idea/symfony2plugin/config/yaml/inspection/EventMethodCallInspection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
91 changes: 88 additions & 3 deletions ...fr/adrienbrault/idea/symfony2plugin/routing/inspection/DuplicateLocalRouteInspection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -16,6 +34,7 @@
* @author Daniel Espendiller <daniel@espendiller.net>
*/
public class DuplicateLocalRouteInspection extends LocalInspectionTool {
private static final String MESSAGE = "Symfony: Duplicate route name";

@NotNull
@Override
Expand All @@ -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;
Expand All @@ -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;
}
}
}
}
}
}
}
}
}
}
}
21 changes: 20 additions & 1 deletion src/main/java/fr/adrienbrault/idea/symfony2plugin/util/PhpElementsUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions src/main/resources/META-INF/plugin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,10 +284,10 @@
language="PHP"
implementationClass="fr.adrienbrault.idea.symfony2plugin.routing.inspection.PhpRouteMissingInspection"/>

<localInspection groupPath="Symfony" shortName="Symfony2YamlRouteDuplicateInspection" displayName="Duplicate Route"
<localInspection groupPath="Symfony" shortName="DuplicateLocalRouteInspection" displayName="Duplicate Route"
groupName="Route"
enabledByDefault="true" level="WARNING"
language="yaml"
language=""
implementationClass="fr.adrienbrault.idea.symfony2plugin.routing.inspection.DuplicateLocalRouteInspection"/>

<localInspection groupPath="Symfony" shortName="Symfony2YamlDuplicateServiceKeyInspection" displayName="Duplicate Key"
Expand Down
6 changes: 6 additions & 0 deletions src/main/resources/inspectionDescriptions/DuplicateLocalRouteInspection.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<html>
<body>
Duplicate routes are defined in same scope where only one overwrites the other
<!-- tooltip end -->
</body>
</html>
94 changes: 91 additions & 3 deletions ...rault/idea/symfony2plugin/tests/routing/inspection/DuplicateLocalRouteInspectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,31 @@
* @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", "" +
"foo:\n" +
" car: foo\n" +
"f<caret>oo:\n" +
" car: foo\n",
"Symfony: Duplicate key"
"Symfony: Duplicate route name"
);

assertLocalInspectionContains("routing.yml", "" +
"fo<caret>o:\n" +
" car: foo\n" +
"foo:\n" +
" car: foo\n",
"Symfony: Duplicate key"
"Symfony: Duplicate route name"
);

assertLocalInspectionNotContains("routing.yml", "" +
Expand All @@ -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", "<?php\n" +
"\n" +
"use Symfony\\Component\\Routing\\Annotation\\Route;\n" +
"\n" +
"class FooController\n" +
"{\n" +
" #[Route(name: 'foobar<caret>_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", "<?php\n" +
"\n" +
"use Symfony\\Component\\Routing\\Annotation\\Route;\n" +
"\n" +
"class FooController\n" +
"{\n" +
" #[Route(name: 'foobar<caret>_index')]\n" +
" public function index() {}\n" +
"\n" +
" #[Route(name: 'foobar_index')]\n" +
" public function foo() {}\n" +
"}\n",
"Symfony: Duplicate route name"
);

assertLocalInspectionNotContains("controller.php", "<?php\n" +
"\n" +
"use Symfony\\Component\\Routing\\Annotation\\Route;\n" +
"\n" +
"class FooController\n" +
"{\n" +
" #[Route(name: 'foobar<caret>_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", "<?php\n" +
"\n" +
"use Symfony\\Component\\Routing\\Annotation\\Route;\n" +
"\n" +
"class FooController\n" +
"{\n" +
" /**\n" +
" * @Route(name=\"test\") \n" +
" */\n" +
" public function index() {}\n" +
"\n" +
" /**\n" +
" * @Route(name=\"te<caret>st\")\n" +
" */\n" +
" public function index2()\n" +
" {\n" +
" }\n" +
"}\n",
"Symfony: Duplicate route name"
);
}

public void testDuplicateRoutingKeysForXml() {
assertLocalInspectionContains("routing.xml", "" +
"<routes>\n" +
" <route id=\"blog_list\" path=\"/blog\"/>\n" +
" <route id=\"blog<caret>_list\" path=\"/blog1\"/>\n" +
"</routes>",
"Symfony: Duplicate route name"
);
}
}
19 changes: 19 additions & 0 deletions ...t/idea/symfony2plugin/tests/routing/inspection/fixtures/DuplicateLocalRouteInspection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

namespace Symfony\Component\Routing\Annotation
{
/**
* Annotation class for @Route().
*
* @Annotation
* @NamedArgumentConstructor
* @Target({"CLASS", "METHOD"})
*
* @author Fabien Potencier <fabien@symfony.com>
* @author Alexander M. Turek <me@derrabus.de>
*/
#[\Attribute(\Attribute::IS_REPEATABLE | \Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD)]
class Route
{
}
}