diff --git a/resources/META-INF/plugin.xml b/resources/META-INF/plugin.xml index 033813618..709645aa6 100644 --- a/resources/META-INF/plugin.xml +++ b/resources/META-INF/plugin.xml @@ -204,6 +204,13 @@ enabledByDefault="true" level="ERROR" implementationClass="com.magento.idea.magento2plugin.inspections.xml.AclResourceXmlInspection"/> + + diff --git a/resources/magento2/inspection.properties b/resources/magento2/inspection.properties index 881c7ab50..d12e724f1 100644 --- a/resources/magento2/inspection.properties +++ b/resources/magento2/inspection.properties @@ -20,5 +20,9 @@ inspection.observer.disabledObserverDoesNotExist=This observer does not exist to inspection.cache.disabledCache=Cacheable false attribute on the default layout will disable cache site-wide inspection.moduleDeclaration.warning.wrongModuleName=Provided module name "{0}" does not match expected "{1}" inspection.moduleDeclaration.fix=Fix module name -inspection.aclResource.error.missingAttribute=Attribute "{0}" is required -inspection.aclResource.error.idAttributeCanNotBeEmpty=Attribute value "{0}" can not be empty +inspection.error.missingAttribute=Attribute "{0}" is required +inspection.error.idAttributeCanNotBeEmpty=Attribute value "{0}" can not be empty +inspection.warning.class.does.not.exist=The class "{0}" does not exist +inspection.warning.method.does.not.exist=The method "{0}" does not exist in the service class +inspection.warning.method.should.have.public.access=The method "{0}" should have public access +inspection.warning.method.should.have.public.access.fix=Change the method access diff --git a/src/com/magento/idea/magento2plugin/MagentoIcons.java b/src/com/magento/idea/magento2plugin/MagentoIcons.java index 259ea81fb..172a8ec79 100644 --- a/src/com/magento/idea/magento2plugin/MagentoIcons.java +++ b/src/com/magento/idea/magento2plugin/MagentoIcons.java @@ -1,13 +1,15 @@ -/** +/* * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ + package com.magento.idea.magento2plugin; import com.intellij.openapi.util.IconLoader; -import javax.swing.*; +import javax.swing.Icon; +@SuppressWarnings({"PMD.ClassNamingConventions"}) public class MagentoIcons { - public static final Icon WEB_API = IconLoader.getIcon("/icons/webapi.png"); - public static final Icon MODULE = IconLoader.getIcon("/icons/module.png"); + public static final Icon WEB_API = IconLoader.getIcon("/icons/webapi.png", MagentoIcons.class); + public static final Icon MODULE = IconLoader.getIcon("/icons/module.png", MagentoIcons.class); } diff --git a/src/com/magento/idea/magento2plugin/inspections/php/PluginInspection.java b/src/com/magento/idea/magento2plugin/inspections/php/PluginInspection.java index c1e5f98bd..06f485176 100644 --- a/src/com/magento/idea/magento2plugin/inspections/php/PluginInspection.java +++ b/src/com/magento/idea/magento2plugin/inspections/php/PluginInspection.java @@ -21,6 +21,7 @@ import com.magento.idea.magento2plugin.bundles.InspectionBundle; import com.magento.idea.magento2plugin.inspections.php.util.PhpClassImplementsInterfaceUtil; import com.magento.idea.magento2plugin.inspections.php.util.PhpClassImplementsNoninterceptableInterfaceUtil; +import com.magento.idea.magento2plugin.magento.files.AbstractPhpFile; import com.magento.idea.magento2plugin.magento.files.Plugin; import com.magento.idea.magento2plugin.magento.packages.MagentoPhpClass; import com.magento.idea.magento2plugin.magento.packages.Package; @@ -166,7 +167,7 @@ private void checkTargetMethod( ProblemHighlightType.ERROR ); } - if (!targetMethod.getAccess().toString().equals(Plugin.PUBLIC_ACCESS)) { + if (!targetMethod.getAccess().toString().equals(AbstractPhpFile.PUBLIC_ACCESS)) { problemsHolder.registerProblem( pluginMethod.getNameIdentifier(), inspectionBundle.message("inspection.plugin.error.nonPublicMethod"), diff --git a/src/com/magento/idea/magento2plugin/inspections/xml/AclResourceXmlInspection.java b/src/com/magento/idea/magento2plugin/inspections/xml/AclResourceXmlInspection.java index 78bedca70..f6ac23741 100644 --- a/src/com/magento/idea/magento2plugin/inspections/xml/AclResourceXmlInspection.java +++ b/src/com/magento/idea/magento2plugin/inspections/xml/AclResourceXmlInspection.java @@ -55,7 +55,7 @@ public void visitXmlTag(final XmlTag xmlTag) { problemsHolder.registerProblem( identifier, inspectionBundle.message( - "inspection.aclResource.error.idAttributeCanNotBeEmpty", + "inspection.error.idAttributeCanNotBeEmpty", "id" ), ProblemHighlightType.WARNING @@ -80,7 +80,7 @@ public void visitXmlTag(final XmlTag xmlTag) { problemsHolder.registerProblem( identifier, inspectionBundle.message( - "inspection.aclResource.error.missingAttribute", + "inspection.error.missingAttribute", "title" ), ProblemHighlightType.WARNING diff --git a/src/com/magento/idea/magento2plugin/inspections/xml/WebApiServiceInspection.java b/src/com/magento/idea/magento2plugin/inspections/xml/WebApiServiceInspection.java new file mode 100644 index 000000000..01cbeaadf --- /dev/null +++ b/src/com/magento/idea/magento2plugin/inspections/xml/WebApiServiceInspection.java @@ -0,0 +1,159 @@ +/* + * Copyright © Magento, Inc. All rights reserved. + * See COPYING.txt for license details. + */ + +package com.magento.idea.magento2plugin.inspections.xml; + +import com.intellij.codeInspection.ProblemHighlightType; +import com.intellij.codeInspection.ProblemsHolder; +import com.intellij.codeInspection.XmlSuppressableInspectionTool; +import com.intellij.psi.PsiElementVisitor; +import com.intellij.psi.PsiFile; +import com.intellij.psi.XmlElementVisitor; +import com.intellij.psi.xml.XmlAttribute; +import com.intellij.psi.xml.XmlTag; +import com.jetbrains.php.PhpIndex; +import com.jetbrains.php.lang.psi.elements.Method; +import com.jetbrains.php.lang.psi.elements.PhpClass; +import com.magento.idea.magento2plugin.bundles.InspectionBundle; +import com.magento.idea.magento2plugin.inspections.xml.fix.MethodNotPublicAccessQuickFix; +import com.magento.idea.magento2plugin.magento.files.ModuleWebApiXmlFile; +import java.util.Collection; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +@SuppressWarnings({"PMD.ExcessiveMethodLength", "PMD.NPathComplexity"}) +public class WebApiServiceInspection extends XmlSuppressableInspectionTool { + + @NotNull + @Override + public PsiElementVisitor buildVisitor( + final @NotNull ProblemsHolder problemsHolder, + final boolean isOnTheFly + ) { + return new XmlElementVisitor() { + private final InspectionBundle inspectionBundle = new InspectionBundle(); + + @Override + public void visitXmlTag(final XmlTag xmlTag) { + final PsiFile file = xmlTag.getContainingFile(); + final String filename = file.getName(); + if (!filename.equals(ModuleWebApiXmlFile.FILE_NAME)) { + return; + } + + if (!xmlTag.getName().equals(ModuleWebApiXmlFile.SERVICE_TAG_NAME)) { + return; + } + + //Check whether the class attribute is not empty + final XmlAttribute classAttribute = xmlTag.getAttribute( + ModuleWebApiXmlFile.CLASS_ATTR + ); + if (classAttribute == null) { + return; + } + final String classFqn = classAttribute.getValue(); + if (classFqn == null || classFqn.isEmpty()) { + problemsHolder.registerProblem( + classAttribute, + inspectionBundle.message( + "inspection.error.idAttributeCanNotBeEmpty", + ModuleWebApiXmlFile.CLASS_ATTR + ), + ProblemHighlightType.WARNING + ); + + return; + } + + //Check whether the class exists + final PhpIndex phpIndex = PhpIndex.getInstance( + problemsHolder.getProject() + ); + @NotNull final Collection classes = phpIndex.getClassesByFQN(classFqn); + @NotNull final Collection interfaces = phpIndex + .getInterfacesByFQN(classFqn); + if (classes.isEmpty() && interfaces.isEmpty()) { + problemsHolder.registerProblem( + classAttribute, + inspectionBundle.message( + "inspection.warning.class.does.not.exist", + classFqn + ), + ProblemHighlightType.WARNING + ); + return; + } + + //Check whether the method attribute is not empty + final XmlAttribute methodAttribute = xmlTag.getAttribute( + ModuleWebApiXmlFile.METHOD_ATTR + ); + if (methodAttribute == null) { + return; + } + final String methodName = methodAttribute.getValue(); + if (methodName == null || methodName.isEmpty()) { + problemsHolder.registerProblem( + classAttribute, + inspectionBundle.message( + "inspection.error.idAttributeCanNotBeEmpty", + ModuleWebApiXmlFile.METHOD_ATTR + ), + ProblemHighlightType.WARNING + ); + + return; + } + + //Check whether method exists + Method targetMethod = findTargetMethod(classes, methodName); + if (targetMethod == null) { + targetMethod = findTargetMethod(interfaces, methodName); + } + if (targetMethod == null && methodAttribute.getValueElement() != null) { + problemsHolder.registerProblem( + methodAttribute.getValueElement(), + inspectionBundle.message( + "inspection.warning.method.does.not.exist", + methodName + ), + ProblemHighlightType.WARNING + ); + + return; + } + + //API method should have public access + if (targetMethod.getAccess() != null && !targetMethod.getAccess().isPublic()) { + problemsHolder.registerProblem( + methodAttribute, + inspectionBundle.message( + "inspection.warning.method.should.have.public.access", + methodName + ), + ProblemHighlightType.WARNING, + new MethodNotPublicAccessQuickFix(targetMethod) + ); + } + } + + @Nullable + private Method findTargetMethod( + final Collection classes, + final String methodName + ) { + for (final PhpClass phpClass : classes) { + for (final Method method : phpClass.getMethods()) { + if (method.getName().equals(methodName)) { + return method; + } + } + } + return null; + } + }; + } +} diff --git a/src/com/magento/idea/magento2plugin/inspections/xml/fix/MethodNotPublicAccessQuickFix.java b/src/com/magento/idea/magento2plugin/inspections/xml/fix/MethodNotPublicAccessQuickFix.java new file mode 100644 index 000000000..6840d8b17 --- /dev/null +++ b/src/com/magento/idea/magento2plugin/inspections/xml/fix/MethodNotPublicAccessQuickFix.java @@ -0,0 +1,41 @@ +/* + * Copyright © Magento, Inc. All rights reserved. + * See COPYING.txt for license details. + */ + +package com.magento.idea.magento2plugin.inspections.xml.fix; + +import com.intellij.codeInspection.LocalQuickFix; +import com.intellij.codeInspection.ProblemDescriptor; +import com.intellij.openapi.project.Project; +import com.jetbrains.php.lang.inspections.quickfix.PhpChangeMethodModifiersQuickFix; +import com.jetbrains.php.lang.psi.elements.Method; +import com.jetbrains.php.lang.psi.elements.PhpModifier; +import com.magento.idea.magento2plugin.bundles.InspectionBundle; +import org.jetbrains.annotations.NotNull; + +public class MethodNotPublicAccessQuickFix implements LocalQuickFix { + private final InspectionBundle inspectionBundle = new InspectionBundle(); + private final Method method; + + public MethodNotPublicAccessQuickFix(final Method method) { + this.method = method; + } + + @NotNull + @Override + public String getFamilyName() { + return inspectionBundle.message("inspection.warning.method.should.have.public.access.fix"); + } + + @Override + public void applyFix( + final @NotNull Project project, + final @NotNull ProblemDescriptor descriptor + ) { + PhpChangeMethodModifiersQuickFix.changeMethodModifier( + this.method, + PhpModifier.PUBLIC_IMPLEMENTED_DYNAMIC + ); + } +} diff --git a/src/com/magento/idea/magento2plugin/magento/files/AbstractPhpFile.java b/src/com/magento/idea/magento2plugin/magento/files/AbstractPhpFile.java index 5de4b9341..b8a968bdb 100644 --- a/src/com/magento/idea/magento2plugin/magento/files/AbstractPhpFile.java +++ b/src/com/magento/idea/magento2plugin/magento/files/AbstractPhpFile.java @@ -13,6 +13,9 @@ public abstract class AbstractPhpFile implements ModuleFileInterface { public static final String FILE_EXTENSION = "php"; + + public static final String PUBLIC_ACCESS = "public"; + private final String moduleName; private final String className; private NamespaceBuilder namespaceBuilder; diff --git a/src/com/magento/idea/magento2plugin/magento/files/ModuleWebApiXmlFile.java b/src/com/magento/idea/magento2plugin/magento/files/ModuleWebApiXmlFile.java index 880019389..3b5684913 100644 --- a/src/com/magento/idea/magento2plugin/magento/files/ModuleWebApiXmlFile.java +++ b/src/com/magento/idea/magento2plugin/magento/files/ModuleWebApiXmlFile.java @@ -13,6 +13,9 @@ public final class ModuleWebApiXmlFile implements ModuleFileInterface { public static final String FILE_NAME = "webapi.xml"; public static final String TEMPLATE = "Magento Web Api XML"; public static final String DECLARATION_TEMPLATE = "Magento Web Api Declaration"; + public static final String SERVICE_TAG_NAME = "service"; + public static final String CLASS_ATTR = "class"; + public static final String METHOD_ATTR = "method"; @Override public String getFileName() { diff --git a/src/com/magento/idea/magento2plugin/magento/files/Plugin.java b/src/com/magento/idea/magento2plugin/magento/files/Plugin.java index 6919d9b5e..759702f74 100644 --- a/src/com/magento/idea/magento2plugin/magento/files/Plugin.java +++ b/src/com/magento/idea/magento2plugin/magento/files/Plugin.java @@ -24,9 +24,6 @@ public enum PluginType { around//NOPMD } - //allowed methods access type - public static final String PUBLIC_ACCESS = "public"; - private String fileName; /** diff --git a/src/com/magento/idea/magento2plugin/util/magento/plugin/IsPluginAllowedForMethodUtil.java b/src/com/magento/idea/magento2plugin/util/magento/plugin/IsPluginAllowedForMethodUtil.java index 95c1bbd73..a591a5770 100644 --- a/src/com/magento/idea/magento2plugin/util/magento/plugin/IsPluginAllowedForMethodUtil.java +++ b/src/com/magento/idea/magento2plugin/util/magento/plugin/IsPluginAllowedForMethodUtil.java @@ -6,7 +6,7 @@ package com.magento.idea.magento2plugin.util.magento.plugin; import com.jetbrains.php.lang.psi.elements.Method; -import com.magento.idea.magento2plugin.magento.files.Plugin; +import com.magento.idea.magento2plugin.magento.files.AbstractPhpFile; import com.magento.idea.magento2plugin.magento.packages.MagentoPhpClass; public final class IsPluginAllowedForMethodUtil { @@ -30,6 +30,6 @@ public static boolean check(final Method targetMethod) { if (targetMethod.isStatic()) { return false; } - return targetMethod.getAccess().toString().equals(Plugin.PUBLIC_ACCESS); + return targetMethod.getAccess().toString().equals(AbstractPhpFile.PUBLIC_ACCESS); } } diff --git a/testData/inspections/xml/WebApiServiceInspection/existentClass/webapi.xml b/testData/inspections/xml/WebApiServiceInspection/existentClass/webapi.xml new file mode 100644 index 000000000..736d24f25 --- /dev/null +++ b/testData/inspections/xml/WebApiServiceInspection/existentClass/webapi.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/testData/inspections/xml/WebApiServiceInspection/existentMethod/webapi.xml b/testData/inspections/xml/WebApiServiceInspection/existentMethod/webapi.xml new file mode 100644 index 000000000..b5bd332c6 --- /dev/null +++ b/testData/inspections/xml/WebApiServiceInspection/existentMethod/webapi.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/testData/inspections/xml/WebApiServiceInspection/notExistentClass/webapi.xml b/testData/inspections/xml/WebApiServiceInspection/notExistentClass/webapi.xml new file mode 100644 index 000000000..0db241805 --- /dev/null +++ b/testData/inspections/xml/WebApiServiceInspection/notExistentClass/webapi.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/testData/inspections/xml/WebApiServiceInspection/notExistentMethod/webapi.xml b/testData/inspections/xml/WebApiServiceInspection/notExistentMethod/webapi.xml new file mode 100644 index 000000000..e327ef2fd --- /dev/null +++ b/testData/inspections/xml/WebApiServiceInspection/notExistentMethod/webapi.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/testData/inspections/xml/WebApiServiceInspection/notPublicMethod/webapi.xml b/testData/inspections/xml/WebApiServiceInspection/notPublicMethod/webapi.xml new file mode 100644 index 000000000..8bdb5e25b --- /dev/null +++ b/testData/inspections/xml/WebApiServiceInspection/notPublicMethod/webapi.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/testData/inspections/xml/WebApiServiceInspection/publicMethod/webapi.xml b/testData/inspections/xml/WebApiServiceInspection/publicMethod/webapi.xml new file mode 100644 index 000000000..26ec40a90 --- /dev/null +++ b/testData/inspections/xml/WebApiServiceInspection/publicMethod/webapi.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/testData/project/magento2/app/code/Foo/Bar/Service/SimpleService.php b/testData/project/magento2/app/code/Foo/Bar/Service/SimpleService.php index 3d7400c50..daadd8cf7 100644 --- a/testData/project/magento2/app/code/Foo/Bar/Service/SimpleService.php +++ b/testData/project/magento2/app/code/Foo/Bar/Service/SimpleService.php @@ -7,4 +7,9 @@ public function execute(int $param1, string $param2) { } + + protected function myProtected(int $param1, string $param2) + { + + } } \ No newline at end of file diff --git a/tests/com/magento/idea/magento2plugin/inspections/xml/AclResourceXmlInspectionTest.java b/tests/com/magento/idea/magento2plugin/inspections/xml/AclResourceXmlInspectionTest.java index 93fb3ebcb..c48f3b958 100644 --- a/tests/com/magento/idea/magento2plugin/inspections/xml/AclResourceXmlInspectionTest.java +++ b/tests/com/magento/idea/magento2plugin/inspections/xml/AclResourceXmlInspectionTest.java @@ -27,7 +27,7 @@ public void testAclResourceWithNoTitleShouldHaveWarning() { myFixture.configureByFile(getFixturePath(ModuleAclXml.FILE_NAME)); final String errorMessage = inspectionBundle.message( - "inspection.aclResource.error.missingAttribute", + "inspection.error.missingAttribute", "title" ); @@ -41,7 +41,7 @@ public void testOverrideAclResourceWithNoTitleShouldNotHaveWarning() { myFixture.configureByFile(getFixturePath(ModuleAclXml.FILE_NAME)); final String errorMessage = inspectionBundle.message( - "inspection.aclResource.error.missingAttribute", + "inspection.error.missingAttribute", "title" ); @@ -55,7 +55,7 @@ public void testAclResourceWithEmptyIdShouldHaveWarning() { myFixture.configureByFile(getFixturePath(ModuleAclXml.FILE_NAME)); final String errorMessage = inspectionBundle.message( - "inspection.aclResource.error.idAttributeCanNotBeEmpty", + "inspection.error.idAttributeCanNotBeEmpty", "id" ); diff --git a/tests/com/magento/idea/magento2plugin/inspections/xml/WebApiServiceInspectionTest.java b/tests/com/magento/idea/magento2plugin/inspections/xml/WebApiServiceInspectionTest.java new file mode 100644 index 000000000..81f496bb3 --- /dev/null +++ b/tests/com/magento/idea/magento2plugin/inspections/xml/WebApiServiceInspectionTest.java @@ -0,0 +1,126 @@ +/* + * Copyright © Magento, Inc. All rights reserved. + * See COPYING.txt for license details. + */ + +package com.magento.idea.magento2plugin.inspections.xml; + +import com.magento.idea.magento2plugin.magento.files.ModuleWebApiXmlFile; + +public class WebApiServiceInspectionTest extends InspectionXmlFixtureTestCase { + + private static final String CLASS_DOES_NOT_EXIST = + "inspection.warning.class.does.not.exist"; + + private static final String METHOD_DOES_NOT_EXIST = + "inspection.warning.method.does.not.exist"; + + private static final String METHOD_SHOULD_HAVE_PUBLIC_ACCESS = + "inspection.warning.method.should.have.public.access"; + + private static final String NOT_EXISTENT_CLASS = + "Not\\Existent\\Class"; + + private static final String NOT_EXISTENT_METHOD = + "notExistent"; + + private static final String NOT_PUBLIC_METHOD = + "myProtected"; + + @Override + public void setUp() throws Exception { + super.setUp(); + myFixture.enableInspections(WebApiServiceInspection.class); + } + + /** + * Inspection highlights warning if the class attribute in the service tag contains + * name of the not existent class. + */ + public void testNotExistentClass() { + configureFixture(); + + final String errorMessage = inspectionBundle.message( + CLASS_DOES_NOT_EXIST, + NOT_EXISTENT_CLASS + ); + + assertHasHighlighting(errorMessage); + } + + /** + * Inspection skips warning if the service class exists. + */ + public void testExistentClass() { + configureFixture(); + + final String errorMessage = inspectionBundle.message( + CLASS_DOES_NOT_EXIST, + NOT_EXISTENT_CLASS + ); + + assertHasNoHighlighting(errorMessage); + } + + /** + * Inspection highlights warning if the method attribute in the service tag contains + * name of the not existent method. + */ + public void testNotExistentMethod() { + configureFixture(); + + final String errorMessage = inspectionBundle.message( + METHOD_DOES_NOT_EXIST, + NOT_EXISTENT_METHOD + ); + + assertHasHighlighting(errorMessage); + } + + /** + * Inspection skips warning if the service method exists. + */ + public void testExistentMethod() { + configureFixture(); + + final String errorMessage = inspectionBundle.message( + METHOD_DOES_NOT_EXIST, + NOT_EXISTENT_METHOD + ); + + assertHasNoHighlighting(errorMessage); + } + + /** + * Inspection highlights warning if the method attribute in the service tag contains + * name of the method with not public access. + */ + public void testNotPublicMethod() { + configureFixture(); + + final String errorMessage = inspectionBundle.message( + METHOD_SHOULD_HAVE_PUBLIC_ACCESS, + NOT_PUBLIC_METHOD + ); + + assertHasHighlighting(errorMessage); + } + + /** + * Inspection skips warning if the service method has public access. + */ + public void testPublicMethod() { + configureFixture(); + + final String errorMessage = inspectionBundle.message( + METHOD_SHOULD_HAVE_PUBLIC_ACCESS, + NOT_PUBLIC_METHOD + ); + + assertHasNoHighlighting(errorMessage); + } + + private void configureFixture() { + myFixture.configureByFile(getFixturePath(ModuleWebApiXmlFile.FILE_NAME)); + } +}