Rfc: inner class3 #4
Conversation
WalkthroughThis update extensively enhances the Zend Engine’s handling of PHP inner classes and namespaces. Several core files have been modified to add new structure members, enum values, and functions for inner class compilation and resolution (including lexical and required scopes). In parallel, the parser has been updated with new grammar rules and modifiers, and opcodes have been extended to support inner classes. Additionally, new namespace management files and functions have been introduced, and Reflection has been expanded with methods to check class visibility and inner class status. Numerous new test cases validate these changes across error messages, inheritance, autoloading, visibility, and return type handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User Code"
participant Parser as "Parser (zend_language_parser.y)"
participant Compiler as "Compiler (zend_compile_class_decl & zend_compile_inner_class_ref)"
participant VM as "Opcode Handler (ZEND_FETCH_INNER_CLASS)"
participant Exec as "Execution Engine (zend_execute.c)"
User->>Parser: Submit code with inner class declarations
Parser->>Compiler: Convert syntax to AST including inner class nodes
Compiler->>VM: Compile inner class with updated scope rules
VM->>Exec: Execute inner class resolution and instantiate class
Exec-->>User: Return inner class instance
sequenceDiagram
participant User as "User Code"
participant NSMgr as "Namespace Manager (zend_namespaces.c)"
participant Lookup as "Scoped Lookup (zend_lookup_class_in_scope)"
User->>NSMgr: Request namespace resolution
NSMgr->>Lookup: Look up class by name within scope
Lookup-->>NSMgr: Return class entry (or fallback if not found)
NSMgr-->>User: Provide resolved class for instantiation
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (16)
tests/classes/inner_classes/access_modifiers_005.phpt (1)
1-14: Minor grammatical error in the expected error message.The test correctly verifies that the
protected(set)modifier cannot be used on inner classes. However, there's a grammatical error in the expected error message.-Fatal error: Cannot use the protected(set) modifier on a inner class in %s on line %d +Fatal error: Cannot use the protected(set) modifier on an inner class in %s on line %dThe indefinite article "a" should be "an" before words that begin with vowel sounds.
tests/classes/inner_classes/reflection_001.phpt (1)
14-15: Mixing reflection class access stylesLine 14 uses the
::classsyntax to get the class name, while line 15 uses a string literal with the full path. For consistency and code maintainability, consider using the same approach for both.$outer = new \ReflectionClass(Outer::class); -$ref = new \ReflectionClass('n\s\Outer:>Middle:>Inner'); +$ref = new \ReflectionClass(Outer::Middle::Inner::class);Also note that the string literal uses
:>syntax, which appears to be a new notation introduced for inner classes. Ensure this is properly documented for PHP developers.tests/classes/inner_classes/inheritance.phpt (1)
1-15: Test needs assertions to verify inheritance behaviorWhile the test correctly defines a complex inheritance structure with inner classes, it lacks any runtime assertions or expected output. The test only verifies that the code compiles without errors.
Consider adding some assertions to verify the inheritance relationships work as expected at runtime:
class Outer { abstract class Other {} class Middle extends Outer:>Other { class Inner1 extends Outer:>Other {} class Inner2 extends Outer:>Middle {} } } + +// Verify inheritance relationships +$inner1 = new Outer:>Middle:>Inner1(); +$inner2 = new Outer:>Middle:>Inner2(); +var_dump($inner1 instanceof Outer:>Other); +var_dump($inner2 instanceof Outer:>Middle); ?> --EXPECT-- +bool(true) +bool(true)Zend/zend_vm_def.h (1)
1801-1902: Double-check null pointer usage forscope->lexical_scopeand consider refactoring private/protected checks into a helper function.At lines 1879 and 1887, the code directly uses
scope->lexical_scopewithout verifying it is non-null, which could lead to undefined behavior ifscope->lexical_scopeis unexpectedly null. Additionally, the repeated logic for handling private vs. protected inner classes might be cleaner as a helper function to improve maintainability.Please verify whether
scope->lexical_scopecan indeed be null under certain edge cases. If so, consider the following fix:- if (scope != NULL && (inner_ce->required_scope == scope || scope->lexical_scope == inner_ce->required_scope)) { + if (scope != NULL && (inner_ce->required_scope == scope || + (scope->lexical_scope && scope->lexical_scope == inner_ce->required_scope))) { // ... } - if (scope != NULL && (instanceof_function(scope, inner_ce->required_scope) - || instanceof_function(scope->lexical_scope, inner_ce->required_scope))) { + if (scope != NULL && (instanceof_function(scope, inner_ce->required_scope) + || (scope->lexical_scope && instanceof_function(scope->lexical_scope, inner_ce->required_scope)))) { // ... }tests/classes/inner_classes/visibility_004.phpt (1)
21-21: Typo in error messageThere appears to be a typo in the expected error message: "visibile" should be "visible".
-Fatal error: Uncaught TypeError: Cannot assign protected Outer:>Inner to higher visibile property Outer::illegal in %s:%d +Fatal error: Uncaught TypeError: Cannot assign protected Outer::Inner to higher visible property Outer::illegal in %s:%dtests/classes/inner_classes/access_modifiers_007.phpt (1)
7-7: Modifier order is unconventionalWhile functionally correct, the order of modifiers is typically "abstract public" rather than "public abstract" in PHP. This is just a stylistic observation.
- public abstract class Inner {} + abstract public class Inner {}Zend/zend_namespaces.c (1)
27-29: Consider concurrency implications of static namespace storage
Storing the global namespace and namespaces hash in static variables can introduce concurrency issues in multi-threaded contexts. Evaluate whether locks or thread-local mechanisms are needed to ensure thread safety.ext/reflection/php_reflection.c (1)
4079-4092: Consider using a more robust check for inner class detection.The current implementation relies on checking for the ":>" substring in the class name. While functional, this string-based approach could be fragile if the naming pattern changes or if this substring could appear in class names for other reasons.
The "todo" comment also suggests this is a temporary solution. A more reliable approach might involve checking a dedicated flag or property on the class entry, if available.
Zend/zend_compile.c (2)
1057-1087: Repeated exception blocks could be consolidated.
Each condition throws azend_ce_compile_error. Consider unifying or factoring out duplicate checks to enhance maintainability.
9184-9227: Naming scheme and flag configuration for nested classes are mostly consistent.
Appending"\"to the parent’s class name differs from the":>"internal notation, which could confuse future maintainers. Consider documenting or unifying the separators.Zend/zend_object_handlers.c (5)
384-409: Watch out for potential infinite loop in lexical scope fallback.These lines introduce a label and a
gototo re-checkscope->lexical_scope, but there's no safeguard ifscope->lexical_scopechains in a cycle. If a class’s lexical scope is inadvertently self-referential, it could cause an infinite loop. Consider adding a condition to detect repeated scopes (or rewriting with a loop that breaks when no further lexical scope is found).Would you like me to help verify whether such a cycle can occur by analyzing all
lexical_scopeassignments within the repository?
1823-1824: Avoid repeating lexical scope fallback logic unnecessarily.Storing
original_scopeand jumping back tocheck_lexical_scopeis consistent with the previous pattern but again risks recursion ifscope->lexical_scopepoints back toscope. Consider adding a visited-set or a loop break condition to prevent infinite loops, or a more direct approach to scope resolution withoutgoto.Also applies to: 1842-1845
1905-1906: Unified approach to scope checks recommended.This code snippet repeats the pattern of falling back to
scope->lexical_scopeviagoto, mirroring the changes above. A single unified function or macro to handle lexical scope resolution with cycle detection could reduce duplication and potential errors.Also applies to: 1913-1915
1994-1995: Consolidate repeated lexical scope resolution.Once again, this block performs the same lexical scope fallback with a
goto. Consolidating this logic into a helper function that checks for cycles would improve maintainability and reduce the chance of hard-to-debug recursion.Also applies to: 2000-2002
2083-2084: Ensure constructor scope logic doesn’t get stuck.Re-checking
scope->lexical_scopehere for constructor access repeats the same pattern. A common approach with an explicit loop or helper function for lexical scope resolution (with cycle detection) would be cleaner and safer than multiplegotoblocks throughout the file.Also applies to: 2088-2090
Zend/zend_language_parser.y (1)
975-977: Revisit attaching flags to$2->attr
Storing the inner class modifiers inattris slightly inconsistent with the approach used elsewhere, where flags are typically passed as the second parameter inzend_ast_create_decl. Refactoring to keep flags in a consistent location would streamline maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (65)
Zend/tests/errmsg/errmsg_027.phpt(1 hunks)Zend/zend.h(1 hunks)Zend/zend_ast.h(1 hunks)Zend/zend_compile.c(12 hunks)Zend/zend_compile.h(3 hunks)Zend/zend_execute.c(2 hunks)Zend/zend_execute_API.c(2 hunks)Zend/zend_language_parser.y(7 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_namespaces.h(1 hunks)Zend/zend_object_handlers.c(7 hunks)Zend/zend_opcode.c(1 hunks)Zend/zend_vm_def.h(3 hunks)Zend/zend_vm_execute.h(24 hunks)Zend/zend_vm_handlers.h(1 hunks)Zend/zend_vm_opcodes.c(3 hunks)Zend/zend_vm_opcodes.h(1 hunks)configure.ac(1 hunks)ext/opcache/zend_persist.c(2 hunks)ext/reflection/php_reflection.c(1 hunks)ext/reflection/php_reflection.stub.php(1 hunks)ext/reflection/php_reflection_arginfo.h(4 hunks)ext/reflection/tests/ReflectionClass_toString_001.phpt(2 hunks)ext/reflection/tests/bug74454.phpt(1 hunks)main/main.c(2 hunks)tests/classes/inner_classes/access_modifiers_001.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_002.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_003.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_004.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_005.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_006.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_007.phpt(1 hunks)tests/classes/inner_classes/autoload_001.phpt(1 hunks)tests/classes/inner_classes/autoload_002.phpt(1 hunks)tests/classes/inner_classes/enum_usage.phpt(1 hunks)tests/classes/inner_classes/errors_001.phpt(1 hunks)tests/classes/inner_classes/errors_002.phpt(1 hunks)tests/classes/inner_classes/inheritance.phpt(1 hunks)tests/classes/inner_classes/inner_classes.inc(1 hunks)tests/classes/inner_classes/interface_usage.phpt(1 hunks)tests/classes/inner_classes/readonly_001.phpt(1 hunks)tests/classes/inner_classes/reflection_001.phpt(1 hunks)tests/classes/inner_classes/return_types_001.phpt(1 hunks)tests/classes/inner_classes/return_types_002.phpt(1 hunks)tests/classes/inner_classes/return_types_003.phpt(1 hunks)tests/classes/inner_classes/return_types_004.phpt(1 hunks)tests/classes/inner_classes/return_types_005.phpt(1 hunks)tests/classes/inner_classes/return_types_006.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_001.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_002.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_003.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_004.phpt(1 hunks)tests/classes/inner_classes/static_resolution.phpt(1 hunks)tests/classes/inner_classes/static_variables.phpt(1 hunks)tests/classes/inner_classes/trait_usage.phpt(1 hunks)tests/classes/inner_classes/visibility_001.phpt(1 hunks)tests/classes/inner_classes/visibility_002.phpt(1 hunks)tests/classes/inner_classes/visibility_003.phpt(1 hunks)tests/classes/inner_classes/visibility_004.phpt(1 hunks)tests/classes/inner_classes/visibility_005.phpt(1 hunks)tests/classes/inner_classes/visibility_006.phpt(1 hunks)tests/classes/inner_classes/visibility_007.phpt(1 hunks)tests/classes/inner_classes/visibility_008.phpt(1 hunks)tests/classes/inner_classes/visibility_009.phpt(1 hunks)tests/classes/inner_classes/visibility_010.phpt(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
main/main.c (2)
Zend/zend_namespaces.h (1)
zend_destroy_namespaces(27-27)Zend/zend_namespaces.c (1)
zend_destroy_namespaces(110-123)
Zend/zend_namespaces.h (1)
Zend/zend_compile.h (1)
name(988-988)
Zend/zend_namespaces.c (4)
Zend/zend_compile.h (2)
name(988-988)zend_initialize_class_data(983-983)Zend/zend_execute_API.c (4)
zend_class_entry(709-721)zend_class_entry(1190-1336)zend_class_entry(1339-1342)zend_class_entry(1345-1360)Zend/zend_compile.c (1)
zend_class_entry(1344-1384)Zend/zend_namespaces.h (2)
zend_resolve_namespace(26-26)zend_destroy_namespaces(27-27)
Zend/zend_vm_execute.h (2)
Zend/zend_vm_def.h (18)
SAVE_OPLINE(30-30)SAVE_OPLINE(89-89)SAVE_OPLINE(148-148)SAVE_OPLINE(224-224)SAVE_OPLINE(234-234)SAVE_OPLINE(282-282)SAVE_OPLINE(324-324)SAVE_OPLINE(500-500)SAVE_OPLINE(580-580)SAVE_OPLINE(660-660)SAVE_OPLINE(725-725)SAVE_OPLINE(807-807)SAVE_OPLINE(846-846)SAVE_OPLINE(885-885)SAVE_OPLINE(938-938)SAVE_OPLINE(991-991)ZEND_VM_NEXT_OPCODE(8149-8149)ZEND_VM_NEXT_OPCODE(8156-8156)Zend/zend_execute.c (1)
zend_swap_operands(4935-4956)
Zend/zend_execute.c (5)
Zend/zend_compile.c (8)
bool(309-323)bool(527-530)bool(2050-2060)bool(2063-2074)bool(2437-2462)bool(9854-9874)bool(9876-9938)bool(9953-9964)Zend/zend_object_handlers.c (3)
bool(259-270)bool(971-976)bool(1591-1615)Zend/zend_ast.h (2)
bool(351-353)bool(355-357)Zend/zend_object_handlers.h (2)
ce(249-249)ce(313-313)Zend/zend_execute.h (1)
zend_verify_property_type_error(528-528)
Zend/zend_compile.c (4)
Zend/zend_ast.h (17)
zv(242-242)zv(243-243)ast(239-239)ast(362-365)ast(362-362)ast(371-375)ast(371-371)ast(377-380)ast(377-377)ast(382-386)ast(382-382)ast(418-424)ast(418-418)type(415-417)type(415-415)zend_ast_get_zval(367-367)name(248-248)Zend/zend_compile.h (3)
type(930-930)type(1008-1008)name(988-988)Zend/zend_namespaces.h (1)
zend_resolve_namespace(26-26)Zend/zend_namespaces.c (1)
zend_resolve_namespace(87-108)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (154)
configure.ac (1)
1760-1760: Addition of zend_namespaces.c to the build systemThis change adds the new
zend_namespaces.csource file to the Zend module in the build system. This file is needed to support the inner classes implementation by providing namespace management functionality.ext/reflection/tests/bug74454.phpt (1)
17-17: Updated error message to reflect the new parser expectationsThe error message has been updated to expect a "class" token instead of a "function" token. This change is consistent with the introduction of inner classes, as the parser's behavior for class declarations has been modified.
Zend/tests/errmsg/errmsg_027.phpt (1)
16-16: Clarified error message for class declarations inside functionsThe error message has been refined to specifically state that class declarations may not be declared inside functions, rather than using the more general "may not be nested" wording. This is an important distinction with the introduction of inner classes, as class nesting is now allowed in some contexts (inner classes) but still prohibited inside function bodies.
tests/classes/inner_classes/access_modifiers_004.phpt (1)
1-14: Added test case for public(set) modifier on inner classesThis test verifies that the
public(set)access modifier cannot be applied to inner classes. The test correctly expects a fatal error when this invalid combination is used. This is part of a comprehensive test suite for inner class access modifiers.tests/classes/inner_classes/errors_001.phpt (2)
6-6: Good test case for validating inner class error handling.The test correctly verifies that attempting to instantiate an inner class when the outer class doesn't exist produces an appropriate fatal error. The
Outer:>Innersyntax appears to be the correct notation for accessing inner classes in PHP.
9-9: Error message provides clear feedback.The error message clearly indicates the issue - the outer class 'Outer' was not found for the inner class instantiation. This helps developers quickly identify and fix the problem.
tests/classes/inner_classes/access_modifiers_003.phpt (2)
6-9: Good test case for static modifier constraint on inner classes.This test correctly verifies that inner classes cannot be declared with the
staticmodifier in PHP. This appears to be an intentional design decision in PHP's implementation of inner classes.
13-13: Clear error message for static modifier restriction.The error message is appropriately descriptive, making it clear to developers that the
staticmodifier isn't allowed on inner classes.tests/classes/inner_classes/simple_declaration_002.phpt (2)
6-10: Good test to verify class declaration constraints.This test correctly verifies that classes cannot be declared inside functions, which is an existing constraint in PHP. This ensures that inner class implementation doesn't inadvertently allow violating this constraint.
13-13: Error message is consistent with PHP's existing behavior.The error message clearly indicates that class declarations are not allowed inside functions, maintaining consistency with PHP's existing error reporting.
tests/classes/inner_classes/access_modifiers_001.phpt (2)
6-9: Good test for access modifier constraints on inner classes.This test correctly verifies that inner classes, like regular classes, cannot have multiple access modifiers. The test ensures that the same language rules apply consistently to inner classes.
13-13: Error message is clear and consistent.The error message appropriately indicates that multiple access type modifiers are not allowed, which is consistent with PHP's handling of regular classes.
tests/classes/inner_classes/autoload_002.phpt (1)
1-17: The autoloading test for inner classes looks well-structured.This test effectively verifies two important aspects of inner class functionality:
- Autoloaders properly work for classes containing inner classes
- Access restrictions on private inner classes are enforced even when autoloaded
The test registers an autoloader that loads the
inner_classes.incfile, attempts to instantiate a private inner class, and correctly expects a fatal error due to the private access restriction.tests/classes/inner_classes/static_resolution.phpt (1)
1-17: Good test for validating static resolution restrictions with inner classes.This test properly verifies that the
statickeyword cannot be used to reference an inner class. This makes sense architecturally since inner classes are designed to be associated with specific instances of the outer class, not with the class itself. Thestatic:>Inner()syntax is correctly rejected with an appropriate error message.Zend/zend_ast.h (1)
136-136: Good addition of AST node type for inner class support.The new
ZEND_AST_INNER_CLASSenum value is correctly placed in the section for AST nodes with two child nodes. This is a necessary addition to enable proper parsing and representation of inner classes in the abstract syntax tree.tests/classes/inner_classes/access_modifiers_002.phpt (1)
1-13: LGTM: Clear test case for invalid inner class syntaxThis test correctly verifies that using a type identifier before the
classkeyword in an inner class declaration results in a syntax error. The error message expectation is appropriate.tests/classes/inner_classes/errors_002.phpt (1)
1-12: LGTM: Appropriate test for non-existent inner classThis test correctly validates the error case when attempting to instantiate an inner class that doesn't exist within the outer class. The error message expectation is accurate.
tests/classes/inner_classes/enum_usage.phpt (1)
1-16: LGTM: Good test for inner class within enumThis test properly validates that inner classes can be defined within PHP enums and accessed using the scope resolution operator. Both instantiation and class existence verification are covered.
I noticed you're using both the
Outer:>InnerandOuter::Innersyntax in the test - this appears intentional to test both access methods.Zend/zend_namespaces.h (1)
1-29: LGTM: Well-structured header file for namespace managementThe header file follows standard Zend Engine conventions with appropriate licensing information and include guards. The two exported functions are clearly declared with their signatures.
tests/classes/inner_classes/return_types_001.phpt (1)
17-18:✅ Verification successful
Expected output uses non-standard inner class notation
The expected output also uses the
:>syntax, which suggests this is indeed an intentional new notation for inner classes rather than a mistake.
🏁 Script executed:
#!/bin/bash # Check if there's any documentation or comments explaining this new syntax rg -i "inner class.*syntax|:>" --type phpLength of output: 43
Verification Complete: Inner class notation is intentional
The execution of the search script did not return any documentation on the new inner class syntax, but the expected output in the test file (showingobject(Outer:>Inner)#1 (0) {}) confirms that the:>notation is indeed intentional. No changes are required as the test output correctly reflects the new inner class notation.tests/classes/inner_classes/reflection_001.phpt (2)
18-22: Good test coverage of reflection methodsThe test appropriately checks various reflection properties including the inner class status and access modifiers.
25-31: LGTM on expected test outputThe expected output correctly verifies all the reflection method results, providing good test coverage for inner class reflection capabilities.
tests/classes/inner_classes/access_modifiers_006.phpt (2)
6-9: Test case for invalid access modifierThis test correctly checks that
private(set)cannot be used as a modifier for inner classes. This is good for validating error handling in the PHP engine.
13-13: Proper error message formattingThe error message uses
%sand%dformatting placeholders which is the standard approach for test files to handle variable file paths and line numbers. This is good practice.tests/classes/inner_classes/simple_declaration_003.phpt (3)
8-16: Good test for deeply nested inner classesThe code defines a three-level nesting of classes (Outer > Middle > Inner) which is a good test case for validating the nested class functionality.
17-17: Inner class instantiation syntaxLine 17 demonstrates the syntax for instantiating a deeply nested inner class using the
:>operator. This appears to be the intended new syntax for inner class references.However, make sure this new syntax is well-documented in PHP documentation to help developers understand the correct usage.
19-20: Expected output validates full class nameThe test correctly verifies that the
__CLASS__constant inside the inner class returns the fully qualified name including namespace and parent classes.tests/classes/inner_classes/visibility_006.phpt (1)
1-29: Test correctly validates inner class scope isolationThis test case properly verifies that inner classes don't have automatic access to private methods of their outer classes. The test demonstrates proper scope boundaries by showing that
Innerclass cannot access the privatetest()method fromOuter.The syntax for accessing inner classes with
Outer:>Middle:>Inneris correctly implemented, and the expected error message appropriately shows that the method is undefined for the inner class.tests/classes/inner_classes/visibility_001.phpt (3)
7-8: Well-structured test for visibility constraintsThe test correctly demonstrates that private inner classes cannot be exposed through higher visibility properties. The visibility constraints are properly enforced.
10-12: Test effectively demonstrates visibility type errorThe test method properly attempts to assign a private inner class instance to a public property, which should trigger a visibility-related TypeError. This effectively validates the PHP type system's enforcement of visibility for inner classes.
21-25: Expected error message correctly specifiedThe expected error message appropriately indicates that a private inner class cannot be assigned to a property with higher visibility, which aligns with PHP's type system and visibility rules.
tests/classes/inner_classes/autoload_001.phpt (3)
5-8: Well-implemented autoloader registrationThe test correctly registers an autoloader function that loads class files from the same directory and provides feedback through console output.
13-15: Expected output assumes correct file structureThe test expects the autoloader to be called with "inner_classes" and then for the Point class to initialize with the given coordinates. Ensure that the directory structure and file naming match the expectations of the autoloader.
10-11:✅ Verification successful
Verify that the inner_classes.inc file exists
The test instantiates
inner_classes:>Point, but it's not immediately clear if the referenced include file exists in the repository.
🏁 Script executed:
#!/bin/bash # Check if inner_classes.inc exists in the tests directory find tests -name "inner_classes.inc" -type f # Alternatively, check the content of the file if it exists if [ -f "tests/classes/inner_classes/inner_classes.inc" ]; then cat "tests/classes/inner_classes/inner_classes.inc" fiLength of output: 359
Final Verification: inner_classes.inc File Exists
I verified that the file exists at
tests/classes/inner_classes/inner_classes.incand that it contains the expected definitions—including the declaration of the nestedPointclass. The file's presence confirms that the instantiation ofinner_classes:>Pointin the test is valid.tests/classes/inner_classes/trait_usage.phpt (3)
6-8: Trait with inner class well definedThe test correctly defines a trait containing an inner class, which is an important test case for the trait usage with inner classes feature.
10-17: Test demonstrates expected trait-inner class behaviorThe test properly demonstrates that:
- Inner classes defined within traits are directly accessible using trait name syntax
- Inner classes are not automatically inherited when using traits in classes
This is important behavior to test as it differs from normal PHP class inheritance.
20-24: Expected output correctly reflects traits and inner classes relationshipThe expected output properly shows that:
Outer:>Innerexists and can be instantiatedFoo:>Innerdoes not exist, confirming that inner classes in traits don't get imported into the using classThis behavior is consistent with PHP's trait usage where traits are used as a composition mechanism rather than inheritance.
tests/classes/inner_classes/readonly_001.phpt (1)
1-24: LGTM: Well-structured test for readonly inner classesThis test correctly verifies that readonly properties in inner classes work as expected. The syntax for accessing inner classes (
Outer:>Inner) is consistent with the implementation, and the test properly confirms that readonly properties cannot be modified after initialization.tests/classes/inner_classes/return_types_004.phpt (1)
1-27: LGTM: Correctly validates visibility constraintsThis test properly verifies that private inner classes cannot be returned from public methods. The expected error message accurately reflects the visibility constraint violation, ensuring that inner class visibility modifiers are enforced properly.
Zend/zend_vm_opcodes.h (1)
294-296: LGTM: Proper opcode addition for inner class supportThe addition of the
ZEND_FETCH_INNER_CLASSopcode and corresponding update toZEND_VM_LAST_OPCODEare correctly implemented. This change follows the existing pattern in the file and is essential for the inner classes feature.ext/opcache/zend_persist.c (2)
1124-1143: Implementation for updating scope entries in shared memory.This new function correctly handles the translation of required and lexical scopes to their shared memory representation. The implementation follows the established pattern in the codebase for shared memory translation.
The code properly checks if the scopes are set before attempting to translate them, and uses the appropriate getter for shared memory access.
1318-1318: Required scope update integrated into class table persistence.Good placement of the new
zend_update_required_scope(ce)call within the class table persistence process. It's correctly positioned after the parent class entry update, ensuring all class entry relationships are properly maintained in shared memory.Zend/zend.h (1)
167-170: New class entry fields to support inner classes.The addition of these three fields to the
_zend_class_entrystructure enables proper management of inner class scope relationships:
required_scope- Indicates the class that must be used to access this classlexical_scope- Tracks the lexical scope of the class for visibility rulesrequired_scope_absolute- Flag to indicate if the required scope is absoluteThe fields are added in a logical location within the structure and follow the existing coding style.
tests/classes/inner_classes/simple_declaration_001.phpt (1)
1-21: Basic inner class declaration and access test.This test effectively validates the core functionality of inner class declarations:
- It verifies inner class existence using string identifiers with the
:>operator- It confirms the
::classconstant works properly with inner classes- It tests nested inner classes (with multiple levels of nesting)
The test correctly sets up the class hierarchy and uses appropriate assertions to verify the expected behavior.
tests/classes/inner_classes/visibility_008.phpt (1)
1-30: Comprehensive test for sibling method access in inner classes.This test effectively validates the visibility rules for inner classes by checking:
- Access to static methods in sibling classes (
Outer:>Middle::test())- Access to protected methods in parent classes from inner classes
- Proper method resolution within the inheritance hierarchy
The test creates a realistic scenario with protected members and inheritance, properly demonstrating that visibility rules are maintained for inner classes.
tests/classes/inner_classes/inner_classes.inc (1)
1-10: LGTM! This is a good test fixture for inner classes.The file correctly defines a
inner_classesclass with a public inner classPointand a private inner classLine. ThePointclass uses constructor property promotion, which is a good practice to avoid boilerplate code.tests/classes/inner_classes/return_types_005.phpt (3)
15-16: Same syntax inconsistency in function return type.The function return type also uses the
:>syntax. This appears to be a consistent pattern for inner class references.
22-26: Test expectation correctly captures the visibility error.The test correctly expects a fatal error when attempting to return a protected inner class from a public method, enforcing proper visibility constraints.
9-10:❓ Verification inconclusive
Syntax inconsistency in inner class reference.
The code uses
:>syntax (e.g.,self:>Inner) rather than the standard::syntax for class reference. Is this intentional for inner classes?
🏁 Script executed:
#!/bin/bash # Searching for other occurrences of this syntax rg -A 1 ":>" --type phpLength of output: 25
Verify inner class syntax in
tests/classes/inner_classes/return_types_005.phptThe snippet at lines 9–10 shows the use of
:>(e.g.self:>Inner) instead of the standard::for class references. This non-standard inner class reference syntax appears isolated (our initial grep produced no further occurrences, possibly due to the file extension) and raises the question of whether this is an intentional deviation from PHP’s typical syntax. Please manually verify whether the use of:>in this context is deliberate. If so, consider adding documentation or comments to clarify its purpose; otherwise, update it to use the conventional::operator.
To help with manual verification, please run the following shell script to inspect the snippet:
#!/bin/bash # Display lines 9-10 from the specified test file to confirm the syntax in use. sed -n '9,10p' tests/classes/inner_classes/return_types_005.phpttests/classes/inner_classes/return_types_003.phpt (3)
9-11: Inner class return type defined with the same:>syntax.The method uses the same
:>syntax for inner class references as seen in the previous file, which seems to be the intended syntax for this feature.
15-18: Proper test for visibility conflict with inherited members.The code correctly tests a scenario where a child class tries to override a protected method to return a protected inner class from the parent, which should cause a visibility conflict.
21-23: Verify this line is reachable before the error.This line attempts to instantiate
Outer:>Innerdirectly, but the expected output doesn't show this line executing before the fatal error. Check if this code is reachable or if the error occurs earlier.Zend/zend_compile.h (3)
896-898: Good addition to the modifier target enum.Adding
ZEND_MODIFIER_TARGET_INNER_CLASSto thezend_modifier_targetenum is necessary to support access modifiers for inner classes.
1023-1025: New constant for outer class fetching.The
ZEND_FETCH_CLASS_OUTERconstant is properly defined within the fetch class types section, maintaining consistency with other class fetch constants.
1064-1066: Added namespace class constant.The
ZEND_NAMESPACE_CLASSconstant is properly defined in the class type section, which will be useful for classifying classes defined within a namespace.Zend/zend_vm_def.h (2)
4532-4549: Confirm handling of private methods returning private classes from different scopes.Currently, this code checks if a public or protected method is returning a private or protected class. However, there is no similar check for private methods returning classes constrained to another private scope. This could allow unintended visibility violations or lead to inconsistent error handling.
Would you like to confirm whether returning a private inner class from a differently scoped private method is adequately handled? If you suspect it isn't, I can propose a targeted fix or test to verify the behavior.
8296-8296: Looks good.The addition of
ZEND_FETCH_INNER_CLASShere for returning azend_class_entrypointer aligns well with the other fetch opcodes. No additional concerns.tests/classes/inner_classes/interface_usage.phpt (2)
6-8: LGTM! Interface with inner class implementation looks good.The test correctly defines an interface containing an inner class, which is a solid addition to PHP's OOP capabilities.
12-12: Implementation class correctly definedThe class implementing the interface is properly defined.
tests/classes/inner_classes/visibility_004.phpt (1)
6-8: Testing protected inner class visibilityThis test correctly defines a protected inner class with a public property that tries to hold an instance of the inner class. The visibility constraints being tested here are essential for maintaining proper encapsulation.
tests/classes/inner_classes/visibility_007.phpt (1)
6-22: Nested class structure with protected method accessThe test correctly verifies that inner classes can access protected methods from outer classes, which is a key feature for inner class implementation.
tests/classes/inner_classes/access_modifiers_007.phpt (1)
6-8: Abstract inner class implementationThe test correctly defines an abstract inner class, which allows for proper inheritance hierarchies within encapsulated contexts.
ext/reflection/tests/ReflectionClass_toString_001.phpt (2)
33-33: Method count updated successfully
No issues found in increasing the listed method count from 64 to 68.
517-544: Validate test coverage for newly introduced reflection methods
Four new reflection methods (isInnerClass,isPrivate,isProtected, andisPublic) have been added. Verify that there is sufficient test coverage to confirm each method’s correctness and behavior across diverse scenarios.tests/classes/inner_classes/return_types_006.phpt (1)
7-10: Confirm validity of the new ':>' inner class syntax
Lines 9 and 13 use the custom:>operator to reference a private inner class. Validate that this syntax operates as intended and receives thorough coverage, since it is non-standard in mainstream PHP.Also applies to: 13-13
Zend/zend_namespaces.c (1)
115-123: Verify memory deallocation of persisted class entries
The code usespemallocto allocate namespace structures. Confirm thatZEND_CLASS_DTOR(passed tozend_hash_init) fully cleans up each entry to avoid persistent memory leaks.Zend/zend_opcode.c (1)
433-436: Proper memory management for inner class scope relationships.This change handles releasing references to the required scope of a class during destruction, ensuring proper memory management. For inner classes, the required_scope points to the containing/outer class, and this cleanup prevents memory leaks by decrementing the reference count and clearing the pointer.
main/main.c (2)
69-69: Added necessary header for namespace management.The inclusion of
zend_namespaces.hprovides the necessary function declarations for handling namespaces within the Zend Engine, including thezend_destroy_namespaces()function that will be used later in this file.
1971-1971: Added proper namespace cleanup during request shutdown.This addition properly cleans up namespace resources by calling
zend_destroy_namespaces()as part of the PHP request shutdown sequence. This prevents memory leaks by ensuring that all namespace-related resources (such as the namespace hash table entries and global namespace) are properly released when a request ends.ext/reflection/php_reflection.stub.php (2)
436-436: Added new method to check for inner classes.This method enhances the ReflectionClass API by providing a way to determine if a class is an inner class. This is an important addition that supports the new inner class functionality being implemented.
438-442: Added visibility check methods for class access modifiers.These three methods (
isPrivate(),isProtected(), andisPublic()) extend the ReflectionClass API to determine a class's visibility, which is particularly important for inner classes that can now have access modifiers. These methods parallel the existing visibility methods already available for methods and properties in the Reflection API.tests/classes/inner_classes/visibility_009.phpt (4)
1-3: Test case for deeply nested property visibility.This test appropriately describes its purpose of testing property visibility within deeply nested inner classes.
6-25: Proper implementation of nested inner class structure.The test defines a multi-level nested class structure (Outer, Middle, Inner) with private properties at each level. The Inner::test() method demonstrates accessing private properties of outer classes, showcasing the visibility rules for inner classes.
Note the use of the new syntax
:>for accessing inner classes, which appears to be the chosen notation for inner class references.
26-26: Test invocation using full inner class path.This line correctly invokes the test method using the full path notation with
:>separators to access the deeply nested inner class method.
28-38: Expected output correctly shows property visibility in dumps.The expected output verifies that:
- Static property assignments work correctly
- Object dumps correctly show the private property names with their full class path
- Inner class property access respects the expected visibility rules
This effectively tests the property visibility behavior with inner classes.
tests/classes/inner_classes/visibility_003.phpt (4)
1-3: Test case for accessing outer class properties from inner classes.This test is appropriately labeled to test an important aspect of inner class functionality: the ability of inner classes to access properties of their outer class.
6-17: Implementation of inner class with privileged outer class access.This test correctly:
- Defines an Outer class with a private inner class
- Demonstrates that the Inner class can access private members of its enclosing Outer class
- Shows how to instantiate an inner class from within its outer class
The
private class Innersyntax is notable as it demonstrates that inner classes can have visibility modifiers, and the test shows that a private inner class can still access its outer class's private members.
19-22: Tests inner class instantiation and property assignment.This code creates an Outer instance, calls its test method (which in turn creates an Inner instance), and verifies that the Inner instance is correctly assigned to the private property of Outer.
25-30: Expected output verifies private property access.The expected output correctly shows the private property "illegal" containing an instance of the Inner class, demonstrating that the property was successfully set by the inner class method, confirming proper inner-to-outer class access privileges.
tests/classes/inner_classes/static_variables.phpt (3)
6-15: Inner class hierarchy looks well-defined.The test properly establishes a three-level class hierarchy with
Outer,Middle, andInnerclasses. Each inner class correctly defines both a constant and a static variable, which is essential for testing access to these elements.
24-30: Expected output matches inner class access patterns.The expected output correctly validates that both the class names and static members can be accessed using the new inner class syntax. The test covers both constants and static variables.
17-22:✅ Verification successful
Verify the new
:>syntax for inner class access.The test demonstrates a new syntax
:>for accessing inner classes, which differs from PHP's traditional namespace separator\. This is an important feature to validate.
🏁 Script executed:
#!/bin/bash # Check if this syntax is documented in other files grep -r "inner class" --include="*.md" --include="*.txt" . grep -r ":>" --include="*.c" --include="*.h" --include="*.y" Zend/Length of output: 1096
Verified the implementation of the new
:>inner class access syntax.The grep results confirm that the
:>syntax is used and documented in several core Zend files (e.g.,Zend/zend_compile.c,Zend/zend_vm_def.h, andZend/zend_vm_execute.h). The test intests/classes/inner_classes/static_variables.phptcorrectly demonstrates the intended behavior in line with these internal references.tests/classes/inner_classes/return_types_002.phpt (4)
6-8: Private inner class visibility is properly tested.The test correctly defines a private inner class, which is essential for testing visibility restrictions.
9-15: Method return types using inner class notation are well-tested.The test uses the new
self:>Innerreturn type syntax and demonstrates encapsulation by providing a public accessor method that returns instances of the private inner class.
18-23: Inheritance behavior with inner classes is properly tested.The subclass attempts to access the parent's private inner class in two ways:
- Indirectly through
parent::getInner2()(which works)- Directly with
new parent:>Inner()(which correctly fails)This effectively tests visibility boundaries.
28-32: Error handling for private inner class access is correct.The test appropriately expects a fatal error when attempting to directly access a private inner class from outside its containing class, which correctly validates the encapsulation of inner classes.
ext/reflection/php_reflection.c (3)
4095-4104: Implementation looks correct for private class checking.The method checks for both
required_scopeandrequired_scope_absolutebeing set, which appears to be the correct way to determine if a class is private based on the inner class implementation in PHP.
4107-4115: Implementation looks correct for protected class checking.The method checks if
required_scopeis set andrequired_scope_absoluteis not set, which aligns with how protected visibility is implemented for inner classes.
4118-4126: Implementation looks correct for public class checking.The method properly checks if
required_scopeis not set, which would indicate the class has public visibility.ext/reflection/php_reflection_arginfo.h (2)
369-375: New reflection methods added for inner class support.These four new methods for
ReflectionClassenable introspection of inner classes and their visibility modifiers. They all return boolean values and will be useful for examining the structure and accessibility of inner classes.
858-861: Method declarations added to the ReflectionClass implementation.These new method declarations correspond to the arginfo declarations above and complete the implementation. The methods allow inspecting whether a class is an inner class and what visibility modifier (public, protected, private) it has.
tests/classes/inner_classes/visibility_005.phpt (3)
6-22: Test for inner class access to outer private methods.This test verifies that inner classes have access to private methods of their containing classes. The nested structure (Outer > Middle > Inner) demonstrates inheritance of visibility across multiple levels.
16-16: Syntax for accessing outer scope private methods.The
:>operator is used to access enclosing class scopes, allowing the inner class to call private methods it would otherwise have no access to. This is a key feature of inner classes.
23-23: New syntax for instantiating inner classes.The chain of
:>operators defines the path to the inner class through its containing classes. This is consistent with how Java and other languages with inner class support work.Zend/zend_vm_opcodes.c (1)
25-25: Added new ZEND_FETCH_INNER_CLASS opcode.A new opcode has been added to support fetching inner classes. The arrays for opcode names and flags have been expanded from 210 to 211 elements to accommodate this addition. The opcode flag
0x00040307defines the behavior characteristics of this opcode during execution.Also applies to: 236-236, 239-239, 450-450
tests/classes/inner_classes/visibility_010.phpt (3)
6-14: User class with private constructor and property visibility modifiers.The
Userclass demonstrates the builder pattern with inner classes. It declares properties with the newprivate(set)visibility modifier, indicating they can be read publicly but only set privately. The private constructor ensures instances must be created through the builder.
15-29: Builder inner class implementation.The
Builderclass is defined aspublic readonly final, showcasing multiple class modifiers. It implements a fluent interface with methods that return new builder instances, eventually creating the outerUserclass. This demonstrates proper encapsulation while providing a clean API.
32-33: Demonstration of builder pattern with inner classes.This creates an instance of the inner
Builderclass via the new syntax, builds aUserwith a fluent interface, and displays the result. It effectively shows how inner classes can provide cleaner implementations of common patterns like Builder.tests/classes/inner_classes/visibility_002.phpt (5)
1-31: Test structure validates inner class access to outer class private members.The test case correctly validates that inner classes can access private variables of their containing outer class, which is a common behavior in languages with inner class support. The test structure follows proper PHPT format with clear test description, code section, and expected output.
6-12: Inner class implementation follows expected encapsulation rules.The implementation correctly demonstrates that inner classes should have access to private members of their containing class. This is a fundamental aspect of inner class design, as inner classes are conceptually part of their outer class's implementation and should have complete access to it.
12-12: New type annotation syntax for inner classes is properly used.The property declaration uses the
Outer:>Innertype annotation syntax, which clearly indicates that the variable type is an inner class within the Outer class. This syntax is intuitive and consistent with the hierarchical nature of inner classes.
14-16: Inner class instantiation syntax is clear and concise.The usage of
new Outer:>Inner()to instantiate the inner class from within the outer class is intuitive and follows the same syntax pattern as the type annotation. This consistency helps maintain readability and understanding of the code.
26-30: Expected output correctly verifies object structure.The expected output properly demonstrates that the private member of the outer class has been populated with an instance of the inner class, validating both the access permissions and the correct representation of the nested class structure in var_dump output.
Zend/zend_compile.c (10)
41-41: Header inclusion looks good.
This new include forzend_namespaces.haligns with the added namespace resolution usage below.
897-910: Modifier support appears consistent for inner classes.
AllowingT_READONLY,T_ABSTRACT, andT_FINALforZEND_MODIFIER_TARGET_INNER_CLASSlooks logical. Ensure all call sites and error paths properly handle or reject these modifiers in tandem with other flags.
948-949: Meaningful error message for inner class modifiers.
Labeling the target as “inner class” guarantees clarity when reporting errors. No concerns here.
1798-1829: Nested class name resolution logic seems correct.
This recursive approach to build the full:>-separated name is clear, but verify reference counting carefully to avoid leaks.Also applies to: 1833-1835
1867-1871: Early return for inner class in constant expression resolution.
Storing the resolved string directly intozvis a clean solution.
2924-2949:zend_compile_inner_class_refimplementation is coherent.
The recursion for outer classes and the finalZEND_FETCH_INNER_CLASSopcode emission look well-structured.Also applies to: 2955-2959
3003-3004: Explicit error forstaticon an inner class.
Properly prevents static usage for inner classes and ensures consistent behavior.Also applies to: 3008-3008
7430-7432: Typing support for inner classes is well-integrated.
Expandingzend_typeto return a nested class reference ensures consistent type checks.
9245-9246: Anonymous class scope reset is clear.
Settingrequired_scopeandlexical_scopetoNULLproperly ensures no scope constraints for anonymous classes.
11856-11858: HandlingZEND_AST_INNER_CLASSin the ast switch is appropriate.
Compiling inner classes underZEND_FETCH_CLASS_EXCEPTIONexpands coverage for nested references.tests/classes/inner_classes/simple_declaration_004.phpt (5)
1-3: No concerns with the test header.
6-12: Looks good, but consider additional test coverage.
Including edge cases (e.g. instantiating theMiddleclass from contexts other thantestSelf()) helps ensure robust validation of inner class creation and usage.
14-25: Recommend adding negative or edge-case tests.
Examples might include attempting to extendMiddleincorrectly or verifying behavior whenparent:>Middleis not accessible. This ensures comprehensive test coverage for inheritance and scope resolution.
27-31: Repetitive calls appear intentional.
These lines effectively validate that multiple invocations return distinct objects and behave as expected.
33-41: Expected output is consistent with the tested operations.
No concerns; the captured output matches the intended behavior of nested classes.Zend/zend_execute_API.c (2)
1169-1175: Potential null-pointer risk.
Ifscopesomehow starts asNULLor becomesNULLupon traversal, returningscope->namecould lead to a crash. Please confirm if the caller guaranteesscopeis neverNULL, or add a fail-safe check.
1206-1229: Implementation looks logical; consider broader verification.
Traversing uplexical_scopeto match nested namespaces is sound, but confirm performance and ensure that overshadowing from other namespaces is correctly handled.Zend/zend_vm_execute.h (21)
3313-3313: No issues with new case label.
This addition smoothly integratesZEND_FETCH_INNER_CLASSinto the existing switch structure.
10906-10923: Visibility checks look solid.
Your new return-type visibility checks ensure that a public/protected method can’t return private/protected inner classes (unless allowed). This logic is correct and aligns with the intended language design.
21763-21780: Properly handles return-type checks.
These lines correctly raise errors if a public/protected method tries returning a private/protected inner class. This upholds language constraints effectively.
30257-30276: Return-type visibility checks remain consistent.
No functional issues found; this logic parallels earlier sections and properly protects private/protected classes from being returned by a public/protected method.
38167-38186: Visibility checks are properly enforced.
Continues the pattern of safeguarding private/protected inner classes from illicit returns. Implementation appears correct.
50986-51005: Accurate error handling for disallowed class returns.
The error messages and scope checks look good.
58043-58050: Jump table update is aligned with new opcodes.
No issues found with these added entries; references match the newly introduced handlers.
59919-59926: Hybrid VM case additions look correct.
The insertion ofZEND_FETCH_INNER_CLASS_SPEC_CONST_CONSTcase is consistent and properly traced.
61194-61201: Hybrid VM:_TMPVAR_CONSTcase additions look fine.
The case references and tracing statements are appropriate and consistent with the rest of the VM code.
62787-62794: Hybrid VM:_UNUSED_CONSTcase addition.
Similarly, the new case references appear coherent and properly placed.
67250-67254: Function pointer references for new handlers.
No issues found. Each new opcode handler pointer is correctly registered.
68209-68215: Spec rule insertion: no immediate red flags.
The numeric rule references (e.g. 3491) appear to stay consistent with existing patterns.
68243-68249: Additional specs reference.
Continues the same usage of 3491. No sign of collision or logic bug.
68374-68424: Multiple references to 3491.
Even though it’s repeated, this is typical for the specialized rule sets in the VM. Implementation seems correct.
68592-68614: Commutes and specialized rule expansions.
These pattern additions (e.g. 2590, 2615, 2640) match the style used for numeric operation specs.
68619-68635: Spec additions remain consistent.
Rules for 2665, 2690, 2715 appear to correctly follow the established pattern.
68640-68656: Expanded commutative spec logic.
This maintains uniformity with the earlier arithmetic opcode expansions.
68661-68674: Smart-branch spec expansions.
No issues found; the approach aligns with typical VM optimization patterns.
68679-68692: Commutative checks for equality logic.
Code precisely reflects the VM’s approach to is-equal / is-not-identical operations.
68697-68708: Spec expansions for is-equal scenario.
Again, consistent with prior expansions for compare checks.
68713-68840: Extended spec expansions for comparisons and increments.
Adds QM_ASSIGN, FE_FETCH_R, SEND_VAR, etc., following the same numeric rule patterns.Zend/zend_execute.c (3)
1048-1078: Implementation of class visibility checking for inner classes.This new function
zend_check_class_visibilitychecks whether a class is visible in the current context based on its visibility modifiers (public, protected, private). It follows PHP's visibility rules, where public classes are always visible, protected classes are only visible to subclasses, and private classes are only visible within the same class.The implementation is sound and includes appropriate error messages for visibility violations. The error messages are clear and informative, helping developers understand why a particular class cannot be assigned to a property.
1082-1085: Integration of class visibility checks in property type verification.This addition to
i_zend_verify_property_typeadds class visibility verification when assigning objects to typed properties, ensuring that the class of the assigned object has the appropriate visibility in the current context.This is a critical part of the inner class implementation, ensuring type safety with respect to visibility modifiers. The check is properly placed in the property verification flow.
1237-1252: Visibility validation for classes used in type declarations.This change adds verification of class visibility when classes are used as type declarations in method parameters or return types. It prevents using private or protected inner classes outside of their allowed visibility scope.
The implementation correctly checks that:
- Private inner classes can only be used within their defining class
- Protected inner classes can only be used within their defining class or subclasses
- Neither can be used in the global scope
The error messages are clear and descriptive, helping developers understand why a particular class cannot be used in a given context.
Zend/zend_language_parser.y (5)
278-279: Potentially unused grammar type
The newly introducedinner_type_without_staticsymbol appears unreferenced throughout the file. Please confirm whether it is truly needed or if it can be safely removed to reduce future maintenance overhead.
287-288: Validate the addition ofinner_class_statementtooptional_parameter_list
Merging inner class statements into theoptional_parameter_listrule might be intentional but somewhat unusual. Please verify that it doesn’t introduce shift/reduce or reduce/reduce conflicts and ensure it aligns with design requirements.
290-291: No issues found
These definitions cleanly extend existing grammar for modifiers. No concerns at this stage.
629-636: Defaulting to public might need revisiting
When no modifiers are specified, assigningZEND_ACC_PUBLICas the default forinner_class_modifierscould be correct for your use case. However, please confirm that this default matches the intended visibility and behavior for inner classes.
1429-1431: Verify test coverage for dynamic class references
Allowingclass_name_referenceto be a parenthesized expression can be powerful but may require additional tests for edge cases (e.g., nested parentheses or variable-based references).Zend/zend_vm_handlers.h (5)
1375-1378: Introduce opcodes for fetching inner classes
These definitions (ZEND_FETCH_INNER_CLASS_SPEC_*) appear consistent with the new inner-class functionality, matching similar fetch logic already present for other class fetches.
1379-1384: Add specialized opcodes for function calls and counting arrays
New opcodes (ZEND_INIT_FCALL_OFFSET_SPEC_CONST,ZEND_RECV_NOTYPE_SPEC,ZEND_COUNT_ARRAY_SPEC_*, etc.) look well-aligned with existing naming conventions. Ensure these additions have corresponding tests in the engine test suite or relevant integration tests.Would you like to verify coverage of these new opcodes? I can provide a script to grep for their usage in the engine tests if needed.
1389-1650: Implement numerous “no overflow” and integer/double math opcodes
This block introduces multiple arithmetic opcodes (e.g.,ZEND_ADD_LONG_NO_OVERFLOW_SPEC_*,ZEND_SUB_LONG_SPEC_*,ZEND_MUL_DOUBLE_SPEC_*, etc.). They follow the engine’s typical suffix style for optimized math operations. No immediate issues are apparent, but confirm that each is exercised by relevant tests (particularly edge cases, e.g., near integer boundaries).
1651-1799: Add and refine comparison opcodes for long/double checks
These lines define specialized compare operations (ZEND_IS_EQUAL_LONG_SPEC_*,ZEND_IS_SMALLER_DOUBLE_SPEC_*, etc.), improving specificity and performance in the VM. Ensure coverage for tricky comparisons (e.g., float precision and integer overflows).
1800-1877: Expand opcode list for incremental operations, array specialization, and final placeholders
This last range includes further inc/dec opcodes and expansions for array fetch/unset operations (ZEND_FETCH_DIM_R_INDEX_SPEC_…,ZEND_SEND_VAR_SIMPLE_SPEC_…, etc.). The naming remains consistent. Verify that JIT or opcode optimization passes recognize and handle all these new entries.I can provide a script to confirm references in the JIT or optimization layers, if necessary.
| inner_class_statement: | ||
| T_CLASS T_STRING { $<num>$ = CG(zend_lineno); } extends_from implements_list backup_doc_comment '{' class_statement_list '}' | ||
| { $$ = zend_ast_create_decl(ZEND_AST_CLASS, 0, $<num>3, $6, zend_ast_get_str($2), $4, $5, $8, NULL, NULL); } | ||
| ; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating modifier flags within zend_ast_create_decl
Currently, the class declaration is created with hardcoded flags set to 0. If you anticipate needing to support various flags on inner classes, you might consider injecting the computed flags at creation time, as is done with top-level classes, to avoid potential confusion down the road.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Zend/zend_namespaces.c (2)
42-85: Consider Streamlining the Key String Handling.Inside
insert_namespace(), constructing and releasing an extraneedlestring each iteration might be avoided by usingcurrent_ns.sdirectly for both the find and add operations. This helps reduce memory overhead and string duplication.- zend_string *needle = zend_string_init(ZSTR_VAL(current_ns.s), ZSTR_LEN(current_ns.s), 0); - ns = zend_hash_find_ptr(&namespaces, needle); + ns = zend_hash_find_ptr(&namespaces, current_ns.s); ... - zend_hash_add_ptr(&namespaces, current_ns.s, ns); ... - zend_string_release(needle);
105-108: Possible Concurrency Consideration.If multiple threads invoke
zend_resolve_namespacesimultaneously, the initialization ofglobal_namespaceandnamespacesmay not be guarded. Unless the Zend Engine ensures single-threaded initialization, a race condition could occur.Consider adding synchronization or ensuring the engine’s startup routine initializes
global_namespacebefore concurrent accesses.Zend/zend_compile.c (1)
1057-1087: Refactor repeated checks for disallowed modifiersThe repeated
if ((flags & X) || (new_flag & X)) {...}patterns could be consolidated to reduce duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Zend/zend_compile.c(19 hunks)Zend/zend_namespaces.c(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.c (4)
Zend/zend_compile.h (2)
name(988-988)zend_initialize_class_data(983-983)Zend/zend_execute_API.c (4)
zend_class_entry(709-721)zend_class_entry(1190-1336)zend_class_entry(1339-1342)zend_class_entry(1345-1360)Zend/zend_compile.c (1)
zend_class_entry(1344-1384)Zend/zend_namespaces.h (2)
zend_resolve_namespace(26-26)zend_destroy_namespaces(27-27)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (21)
Zend/zend_namespaces.c (3)
87-96: Check Edge Cases for Empty Names.
zend_resolve_namespacehandlesNULLor empty names by returningglobal_namespace. Verify this is desirable for all use-cases (e.g., internal naming collisions or special-case handling).Do you have test coverage ensuring that passing
NULLor""behaves as expected?
30-40:❓ Verification inconclusive
Validate Class Entry Deallocation.
create_namespace()usespemalloc()to allocate azend_class_entry. Ensure that the hashtable's destructor (ZEND_CLASS_DTOR) properly frees all namespace entries. Otherwise, memory allocated here may not be released.Below is a script that searches for
ZEND_CLASS_DTORusage to confirm it callspefreeon each entry:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'ZEND_CLASS_DTOR($_)'Length of output: 862
Attention: Validate Namespace Class Deallocation
The function
create_namespace()usespemalloc()to allocate azend_class_entryfor namespace entries. Please verify that the corresponding namespace destructor—triggered viaZEND_CLASS_DTOR—properly frees these entries withpefree(), ensuring no memory leaks occur. Our automated search did not yield conclusive results (likely due to file encoding issues in some non-related files), so a manual review of the destructor implementation in the Zend class deallocation routines is recommended.
110-123:❓ Verification inconclusive
Confirm No Leaks for Child Namespaces.
During
zend_destroy_namespaces(), child namespace entries are freed via the hashtable destructor. However, onlyzend_string_release(ns->name)is explicitly called in the loop. Ensure thatZEND_CLASS_DTORor similar frees thezend_class_entrystruct to avoid leaked allocations.Use this script to verify the destructor logic:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'zend_hash_init($_, $_, $_, ZEND_CLASS_DTOR, $_)'Length of output: 890
Action Required: Verify Child Namespace Destructor Behavior
The current grep command did not yield the expected confirmation for a destructor (e.g. using ZEND_CLASS_DTOR) on the namespace hash table initialization. Although the loop in
zend_destroy_namespaces()correctly callszend_string_release(ns->name), it does not explicitly free the child namespace struct. Please manually verify that the hash table for namespaces was initialized with an appropriate destructor (or that some equivalent cleanup mechanism exists) to ensure that thezend_class_entrystructs for child namespaces are properly freed. If the hash table was not set up with such a destructor, adjusting its initialization or adding explicit cleanup may be required to avoid memory leaks.
- Confirm that the namespaces table is initialized with a destructor (e.g. via
zend_hash_init(..., ZEND_CLASS_DTOR, ...)) in the relevant initialization code.- If the destructor is missing or another cleanup method is in place, update the code or documentation to reflect the intended memory management.
Zend/zend_compile.c (18)
41-41: Include new header for namespacesNo concerns. This header inclusion is consistent with the new inner class feature that depends on namespace logic.
897-897: Accepting abstract/readonly/final modifiers for inner classesAllowing these modifiers for
ZEND_MODIFIER_TARGET_INNER_CLASSaligns with the engine’s design for class-level flags.Also applies to: 902-902, 910-910
948-949: Adding "inner class" to member stringThis correctly updates the string used in error/context messages to reflect the new target type.
1798-1836: Nested class name resolutionThis recursive approach for resolving nested class names appears logical. To minimize any edge case issues, please confirm that deeply nested classes are tested for both correctness and boundary recursion behavior.
1867-1871: Expanding static resolution for inner classesThis seamlessly integrates with the newly introduced function to handle inner class references.
2924-2924: New helper for compiling an inner class reference
zend_compile_inner_class_refcorrectly emitsZEND_FETCH_INNER_CLASS. The recursion and fallback logic for outer classes looks coherent.Also applies to: 2926-2950, 2955-2959
3003-3004: Disallowing 'static' on an inner classThe compile error logic is consistent with the design choice that inner classes cannot be declared static.
Also applies to: 3008-3009
7430-7432: Typed structure for nested inner classesUsing
ZEND_TYPE_INIT_CLASShere is consistent with how the engine handles class-based type declarations.
9097-9097: Implementing interfaces in nested declarationsThis function signature for
zend_compile_implementsfits well with the engine’s approach to interface handling.
9157-9167: Global queue for deferred inner class declarationsQueuing AST nodes is a valid approach for deferring certain declarations. Just ensure thread safety if parallel compilation arises in the future.
9193-9237: Potential delimiter mismatch ("\\" vs ":>")Here, inner class names are concatenated using backslashes (
"\\\\"), whereas nested class resolution internally uses the":>"delimiter. Please confirm this difference is intended to avoid name collisions or if a unification is needed.
9255-9256: Resetting scope for anonymous or unqualified classesNo issues found. Resets are consistent with the engine’s standard approach for non-nested classes.
9298-9300: Compiling 'implements' for nested or inner classesInvoking
zend_compile_implementsproperly ensures interface references remain consistent for inner classes.
9345-9345: Use ofgotofor finalizing compilation flowThough
gotois often avoided in high-level code, it is quite common and acceptable in the Zend Engine’s compilation pattern.
9354-9354: AnothergotolabelConsistent with surrounding code style and control flow in the engine.
9424-9439: Compiling deferred inner classesInvoking the queued declarations post main compilation is a sensible design. Properly freeing the hashtable prevents leaks.
11721-11724: Deferring const declarations within an active classPlacing constant declarations in the deferred queue maintains consistency when classes are not fully compiled yet.
11886-11888: HandlingZEND_AST_INNER_CLASScase in expression compilationForwarding to
zend_compile_inner_class_refensures we compile the reference properly, respecting fetch flags.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Zend/zend_execute_API.c (1)
1178-1189:⚠️ Potential issueMemory safety bug returning a freed pointer.
Releasing
nameright before returning it leads to a dangling pointer. This creates a use-after-free vulnerability.Apply this fix:
name = zend_string_tolower(name); if (ns && ZSTR_LEN(ns) && ZSTR_LEN(name) > ZSTR_LEN(ns) + 1 && memcmp(ZSTR_VAL(name), ZSTR_VAL(ns), ZSTR_LEN(ns)) == 0 && ZSTR_VAL(name)[ZSTR_LEN(ns)] == '\\') { zend_string *ret = zend_string_init(ZSTR_VAL(name) + ZSTR_LEN(ns) + 1, ZSTR_LEN(name) - ZSTR_LEN(ns) - 1, 0); zend_string_release(name); return ret; } return name;Note: I see this issue was already identified in a previous review and marked as fixed, but the fix doesn't appear to be reflected in this version of the code.
🧹 Nitpick comments (4)
Zend/zend_vm_def.h (1)
4434-4439: Unify repeated error message patterns.
Both lines handle “public method” returning classes with restricted scope. It might be beneficial to abstract these repetitive calls into a small helper to streamline maintenance and ensure consistency.- zend_type_error("Public method %s cannot return private class %s", ...); - zend_type_error("Public method %s cannot return protected class %s", ...); + static void zend_throw_incompatible_return_type_error( + const char *method_type, + zend_function *func, + const char *access_type, + zend_class_entry *ce + ) { + zend_type_error( + "%s method %s cannot return %s class %s", + method_type, + ZSTR_VAL(func->common.function_name), + access_type, + ZSTR_VAL(ce->name) + ); + } + + /* Then call zend_throw_incompatible_return_type_error("Public", EX(func), "private/protected", Z_OBJCE_P(retval_ptr)); */tests/classes/inner_classes/simple_declaration_005.phpt (2)
1-3: Test name and purpose need clarification.The test name indicates this is testing a "failed inheritance" scenario, but the description is minimal. Consider adding more detailed documentation about what specifically is being tested and what the expected behavior should be.
--TEST-- -failed inheritance +Failed inheritance: Inner class resolution in inheritance hierarchy --FILE--
27-31: Redundant test call detected.Lines 29 and 30 are identical, both calling
Outer2::testSelf(). One of these calls appears to be redundant and doesn't add value to the test case.var_dump(Outer::testSelf()); var_dump(Outer2::testParent()); var_dump(Outer2::testSelf()); -var_dump(Outer2::testSelf());Zend/zend_compile.c (1)
9125-9141: Deferred class declaration logic.
Maintaining aninner_class_queueto postpone compilation of nested classes until the primary class is finalized is a good design. Consider adding tests to confirm behavior with multiple nested classes at varying depths.Would you like me to generate test cases or open a new issue to ensure thorough coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
Zend/Optimizer/dfa_pass.c(1 hunks)Zend/zend_API.c(1 hunks)Zend/zend_compile.c(16 hunks)Zend/zend_execute.h(1 hunks)Zend/zend_execute_API.c(3 hunks)Zend/zend_inheritance.c(2 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_namespaces.h(1 hunks)Zend/zend_vm_def.h(1 hunks)Zend/zend_vm_execute.h(5 hunks)ext/reflection/php_reflection.c(1 hunks)tests/classes/.idea/.gitignore(1 hunks)tests/classes/.idea/classes.iml(1 hunks)tests/classes/.idea/deployment.xml(1 hunks)tests/classes/.idea/misc.xml(1 hunks)tests/classes/.idea/modules.xml(1 hunks)tests/classes/.idea/php.xml(1 hunks)tests/classes/.idea/vcs.xml(1 hunks)tests/classes/inner_classes/access_modifiers_007.phpt(1 hunks)tests/classes/inner_classes/autoload_001.phpt(1 hunks)tests/classes/inner_classes/autoload_002.phpt(1 hunks)tests/classes/inner_classes/enum_usage.phpt(1 hunks)tests/classes/inner_classes/errors_001.phpt(1 hunks)tests/classes/inner_classes/errors_002.phpt(1 hunks)tests/classes/inner_classes/inheritance.phpt(1 hunks)tests/classes/inner_classes/interface_usage.phpt(1 hunks)tests/classes/inner_classes/readonly_001.phpt(1 hunks)tests/classes/inner_classes/reflection_001.phpt(1 hunks)tests/classes/inner_classes/return_types_001.phpt(1 hunks)tests/classes/inner_classes/return_types_002.phpt(1 hunks)tests/classes/inner_classes/return_types_003.phpt(1 hunks)tests/classes/inner_classes/return_types_004.phpt(1 hunks)tests/classes/inner_classes/return_types_005.phpt(1 hunks)tests/classes/inner_classes/return_types_006.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_001.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_003.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_004.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_005.phpt(1 hunks)tests/classes/inner_classes/static_variables.phpt(1 hunks)tests/classes/inner_classes/trait_usage.phpt(1 hunks)tests/classes/inner_classes/visibility_001.phpt(1 hunks)tests/classes/inner_classes/visibility_002.phpt(1 hunks)tests/classes/inner_classes/visibility_003.phpt(1 hunks)tests/classes/inner_classes/visibility_004.phpt(1 hunks)tests/classes/inner_classes/visibility_005.phpt(1 hunks)tests/classes/inner_classes/visibility_006.phpt(1 hunks)tests/classes/inner_classes/visibility_007.phpt(1 hunks)tests/classes/inner_classes/visibility_008.phpt(1 hunks)tests/classes/inner_classes/visibility_009.phpt(1 hunks)tests/classes/inner_classes/visibility_010.phpt(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- tests/classes/.idea/modules.xml
- tests/classes/.idea/deployment.xml
- tests/classes/.idea/.gitignore
- tests/classes/.idea/php.xml
- tests/classes/.idea/vcs.xml
- tests/classes/.idea/misc.xml
- tests/classes/.idea/classes.iml
🚧 Files skipped from review as they are similar to previous changes (30)
- Zend/zend_vm_execute.h
- tests/classes/inner_classes/errors_001.phpt
- tests/classes/inner_classes/return_types_001.phpt
- tests/classes/inner_classes/errors_002.phpt
- tests/classes/inner_classes/autoload_001.phpt
- tests/classes/inner_classes/autoload_002.phpt
- tests/classes/inner_classes/reflection_001.phpt
- tests/classes/inner_classes/readonly_001.phpt
- tests/classes/inner_classes/interface_usage.phpt
- tests/classes/inner_classes/return_types_005.phpt
- tests/classes/inner_classes/simple_declaration_003.phpt
- tests/classes/inner_classes/inheritance.phpt
- tests/classes/inner_classes/visibility_006.phpt
- tests/classes/inner_classes/return_types_004.phpt
- tests/classes/inner_classes/enum_usage.phpt
- tests/classes/inner_classes/static_variables.phpt
- tests/classes/inner_classes/return_types_003.phpt
- tests/classes/inner_classes/visibility_008.phpt
- tests/classes/inner_classes/access_modifiers_007.phpt
- tests/classes/inner_classes/visibility_001.phpt
- tests/classes/inner_classes/return_types_006.phpt
- tests/classes/inner_classes/simple_declaration_001.phpt
- tests/classes/inner_classes/visibility_004.phpt
- tests/classes/inner_classes/trait_usage.phpt
- tests/classes/inner_classes/visibility_002.phpt
- tests/classes/inner_classes/return_types_002.phpt
- tests/classes/inner_classes/visibility_007.phpt
- tests/classes/inner_classes/visibility_003.phpt
- tests/classes/inner_classes/visibility_010.phpt
- tests/classes/inner_classes/simple_declaration_004.phpt
🧰 Additional context used
🧬 Code Definitions (3)
Zend/zend_namespaces.c (6)
Zend/zend_namespaces.h (3)
name(26-26)name(28-28)zend_destroy_namespaces(27-27)Zend/zend_execute.h (5)
name(37-37)name(51-51)name(52-52)name(53-53)name(422-422)Zend/zend_compile.h (2)
name(988-988)zend_initialize_class_data(983-983)Zend/zend_execute_API.c (4)
zend_class_entry(710-722)zend_class_entry(1226-1371)zend_class_entry(1374-1377)zend_class_entry(1380-1395)Zend/zend_inheritance.c (2)
zend_class_entry(3520-3792)zend_class_entry(3887-3969)Zend/zend_compile.c (1)
zend_class_entry(1344-1384)
Zend/zend_execute_API.c (3)
Zend/zend_namespaces.h (2)
name(26-26)name(28-28)Zend/zend_execute.h (5)
name(37-37)name(51-51)name(52-52)name(53-53)name(422-422)Zend/zend_compile.h (2)
name(988-988)zend_is_compiling(981-981)
Zend/zend_compile.c (4)
Zend/zend_ast.h (14)
zv(242-242)zv(243-243)ast(239-239)ast(362-365)ast(362-362)ast(371-375)ast(371-371)ast(377-380)ast(377-377)ast(382-386)ast(382-382)ast(418-424)ast(418-418)name(248-248)Zend/zend_namespaces.h (2)
name(26-26)name(28-28)Zend/zend_execute.h (5)
name(37-37)name(51-51)name(52-52)name(53-53)name(422-422)Zend/zend_namespaces.c (1)
zend_resolve_namespace(87-108)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
🔇 Additional comments (40)
ext/reflection/php_reflection.c (4)
4079-4089: Looks good - new method to check if a class is an inner classThis method checks if the class is an inner class by examining if it has a lexical scope that is not a namespace class. The implementation is straightforward and follows the correct pattern for Reflection API methods.
4092-4101: Looks good - method to determine if a class is privateThis method properly determines if a class is private by checking if both
required_scopeandrequired_scope_absoluteproperties are set. The implementation follows the standard pattern for reflection methods.
4104-4111: Looks good - method to determine if a class is protectedThis method correctly identifies if a class is protected by checking if
required_scopeis set whilerequired_scope_absoluteis not set. This is the correct logic for determining protected visibility.
4114-4122: Looks good - method to determine if a class is publicThis method properly checks if a class is public by verifying that
required_scopeis not set. The implementation is correct and consistent with how public visibility would be represented internally.Zend/zend_vm_def.h (2)
4448-4450: Preserve consistent flow of type checks.
The subsequent type check withzend_check_type_slow()gracefully handles type mismatches. Confirm that we don’t introduce additional branching before this check for classes with restricted visibility, ensuring a single, coherent return validation path.
4431-4433:❓ Verification inconclusive
Validate edge cases for private methods returning private classes.
While these checks correctly handle public methods returning private/protected classes, consider similarly stricter checks for private methods returning private classes from a different scope, if such a scenario should be restricted.Would you like me to generate a script to examine other parts of the codebase where private methods return objects, ensuring full consistency?
🏁 Script executed:
#!/bin/bash # Searches for private methods and checks their return statements referencing potential private classes rg -A 10 $'ZEND_ACC_PRIVATE' | rg 'return'Length of output: 5553
Action: Validate stricter scope checks for private method returns
The check in
Zend/zend_vm_def.h(lines 4431–4433) correctly validates public methods returning objects with a required scope. However, our verification (via the executed script) shows several instances where private methods return objects without a corresponding required scope validation. If the design intends to restrict private methods similarly—ensuring that private methods don’t inadvertently return objects from a different scope—please verify the following:
- Confirm whether private methods should undergo the same stricter required-scope check.
- Review other parts of the codebase (e.g., in files like
Zend/zend_compile.candZend/zend_inheritance.c) to ensure consistent application of scope validations.Zend/zend_execute.h (1)
52-52: Well-integrated function for inner class lookup.The new
zend_lookup_class_in_scopefunction is a reasonable addition to support inner class functionality. It extends the existing class lookup functionality by adding scope awareness, which is essential for inner class resolution.Zend/zend_namespaces.h (1)
20-30: Clear and well-structured namespace API definition.The header file follows Zend coding conventions and properly exports the necessary functions for namespace management. The three functions provide a complete API for handling namespaces (resolving, looking up, and cleaning up).
Zend/Optimizer/dfa_pass.c (1)
285-285: Correctly handles inner class scope constraints in type elision.The addition of
!ce->required_scopecondition is necessary to prevent inappropriate type check elision for inner classes. This ensures that visibility rules for inner classes are properly enforced at runtime, even when the optimizer is active.Zend/zend_API.c (1)
1819-1827:✅ Verification successful
Validate behavior for classes requiring a scope when invoked from the global scope.
When
zend_get_executed_scope()returnsNULL, the current logic unconditionally throws an error and aborts object creation. Please confirm that no valid use case needs to allow instantiation from the top-level (global) scope, or consider providing a fallback mechanism if instantiation is intended in certain edge cases.
🏁 Script executed:
#!/bin/bash # Attempt to locate references to required_scope usage to confirm # whether allowing instantiation from the global scope is ever needed. rg "required_scope" -A 5Length of output: 14881
Global Scope Instantiation Behavior Verified
After reviewing the usage of
required_scopeacross the codebase, the current logic inZend/zend_API.c—which throws an error whenzend_get_executed_scope()returnsNULL—is consistent with expectations. No alternative or fallback mechanism is required because instantiation from the global scope is uniformly disallowed for classes with a required scope.
- The checks in
zend_API.calign with similar validations inzend_execute.c,zend_compile.c, and other related modules.- The behavior is consistent across the system: a
NULLexecution scope correctly triggers an error, preventing unsupported instantiation.tests/classes/inner_classes/visibility_009.phpt (3)
6-8: Fine-grained data visibility test is clear.Declaring private properties
$iand$jfor bothOuterandMiddleclasses effectively demonstrates restricted scope in nested scenarios. The approach aligns with the intended test objective.
13-22: Confirm new engine behavior for private static and instance property assignments.By assigning values to
Outer::$jandMiddle::$j, and then updating$foo->i, this test suggests that inner classes can access and modify private members of outer classes. In standard PHP, this would be disallowed. Please verify that this is indeed the intended behavior under the new inner-class visibility rules.
26-26: Good test coverage for final call to inner class.The invocation
Outer\Middle\Inner::test();ensures that the entire nested scope chain is exercised, validating property modifications and the correct output. The test logic is consistent with the corresponding--EXPECT--block.Zend/zend_namespaces.c (1)
123-131:✅ Verification successful
Verify potential double-free in namespace destruction.
Inside
zend_destroy_namespaces(), each class entry’snameis released at lines 125–126, andzend_hash_destroy(&namespaces)is called with aZEND_CLASS_DTORthat may also free the name. Confirm that no double-free occurs during class entry destruction.
🏁 Script executed:
#!/bin/bash # Description: Search the codebase for the ZEND_CLASS_DTOR function to verify it does/doesn't also free `ce->name`. rg 'ZEND_CLASS_DTOR' -A 20Length of output: 8642
Double-Free Verification Passed
After reviewing the usage of
ZEND_CLASS_DTORacross the codebase (notably inZend/zend_namespaces.cand referenced inZend/zend_compile.h), the manual release of each namespace’snamebefore callingzend_hash_destroy()appears intentional. Our search did not reveal any evidence that the destructor (destroy_zend_class) subsequently freesce->nameagain—even though it is used in other contexts without manual string releases. This design is consistent with how other hash tables usingZEND_CLASS_DTORoperate.
- Location:
Zend/zend_namespaces.c(lines 123–131)- Action: No change is needed as the current implementation safely avoids a double-free.
tests/classes/inner_classes/visibility_005.phpt (3)
7-9: Clarify nested scope behavior for private methods.Invoking
test()from outside its class normally triggers a visibility error in standard PHP. The test suggests new scoping rules allow an inner class to access outer private methods. Confirm this is intended.
11-13: Validate static private method access.
private static function test()inMiddlebeing called fromInneris a key scenario that typically would breach scope boundaries in standard PHP. Verify that the new engine’s design explicitly permits this.
15-19: Comprehensive coverage of nested calls.By calling both
Middle::test()and$t->test(), this test ensures that private instance and private static scopes are both exercised from withinInner, which is crucial for checking the correctness of nested visibility rules.tests/classes/inner_classes/simple_declaration_005.phpt (2)
6-12: LGTM: Outer class with inner class declaration.This correctly defines an
Outerclass with a public inner classMiddleand a static method that returns an instance of the inner class.
14-25: Inner class inheritance behavior test case looks good.The code effectively tests how inner classes behave in an inheritance hierarchy.
Outer2extendsOuterand defines its own version of theMiddleinner class, with methods to test both parent and self resolution.Zend/zend_compile.c (15)
41-41: No issues found with the new include.
The addition of#include "zend_namespaces.h"is appropriate for the new functionality.
897-910: Validate final+abstract combination is disallowed.
AllowingT_READONLY,T_ABSTRACT, andT_FINALfor inner classes is consistent with the new design. However, please confirm that contradictory usage such as final+abstract is indeed disallowed to avoid invalid modifiers.
948-949: Clear error message for 'inner class' modifier.
Designating the member as "inner class" clarifies error messages and aligns with the feature expansion.
1057-1087: Restrictions on inner class modifiers appear consistent, but verify conflicts.
Denying multiple access modifiers, static, and 'public(protected/private) set' usage for inner classes is logically coherent with the new constraints. Please confirm that this does not conflict with the logic introduced in hunk 11, which appears to set scope flags for nested classes.
1798-1835: Ensure memory safety in nested class name resolution.
This function properly concatenates the outer and inner class names, releasing the redundantouter_name. Please confirminner_namedoesn’t need manual release and consider tests for multi-level nesting.
1867-1871: Integration with nested class name resolution.
Invokingzend_resolve_nested_class_nameifclass_ast->kind == ZEND_AST_INNER_CLASSappears correctly aligned with the nested class architecture.
2971-2977: Enforcing no static modifier on inner classes.
The compile error for usingstaticwith an inner class is congruent with the restrictions imposed earlier.
7398-7400: Correctly handle inner class AST in type compilation.
Returning a class type for nested classes according tozend_resolve_nested_class_nameis consistent and helps ensure correct typing.
9065-9065: ‘zend_compile_implements’ function made static.
Localizing this function scope is reasonable and keeps the compilation logic self-contained.
9168-9211: Naming and scoping for nested classes.
Concatenating the outer class name with"\\"for the inner class is correct. Please verify there’s no unintentional overlap with the earlier restriction disallowing “public(set)” and “private(set)” usage, since herepropFlagscan include public/private/protected.
9229-9230: Anonymous class scoping adjustments.
Nullifyingce->required_scopeandce->lexical_scopefor anonymous classes is a consistent approach to isolate them from the nested class logic.
9272-9274: Support for implements in nested classes.
Compiling interfaces for the inner class is appropriately handled. No further issues detected.
9299-9313: Processing deferred inner classes after parent class.
Flushing theinner_class_queueensures the child classes are compiled only after the parent’s declaration finalizes. This sequencing looks well-managed.
11684-11687: Defer class declaration within active class scope.
Usingzend_defer_class_declwhenCG(active_class_entry)is set ensures nested classes are processed at the correct stage of compilation.
11697-11700: Defer class declaration for constants inside an active class.
Preventing immediate compilation of class declarations in this context is correct and maintains internal consistency.Zend/zend_inheritance.c (2)
283-288: Well-implemented scope-based class lookup mechanism.This change introduces a fallback mechanism that first attempts to use
zend_lookup_class_in_scopefor scope-aware class resolution, and only if that fails, falls back to the traditionalzend_lookup_class_ex. This approach elegantly integrates inner class support while maintaining backward compatibility.
538-544: Good implementation of scope-based parent class resolution.Similar to the earlier change, this introduces a scope-aware lookup for the parent class that falls back to the traditional name-based lookup if needed. This is crucial for proper inheritance chains with inner classes and ensures correct class hierarchy resolution in a backward-compatible manner.
Zend/zend_execute_API.c (4)
23-23: Good addition of zend_namespaces.h includeThis inclusion is necessary for the new namespace-related functionality added in this file.
1170-1176: The get_namespace_from_scope function correctly retrieves namespace contextThis helper function properly traverses the lexical scope chain until it reaches a namespace boundary or the root scope. It handles the namespace hierarchy correctly for inner class resolution.
1191-1224: Well-structured approach for class lookup in nested scopesThis function implements a hierarchical class resolution approach that correctly traverses the lexical scope chain to find classes defined in parent scopes. The memory management is correct with proper release of all allocated strings.
1242-1264: Good enhancement to class lookup with lexical scope supportThe modifications to
zend_lookup_class_exproperly integrate the new scoped lookup functionality while maintaining backward compatibility. The code correctly handles both runtime and compile-time states to ensure proper class resolution in nested namespaces.This implementation enables inner classes to be properly resolved based on their lexical context.
| } else if (EX(func)->common.fn_flags & ZEND_ACC_PROTECTED) { | ||
| if (Z_OBJCE_P(retval_ptr)->required_scope_absolute && Z_OBJCE_P(retval_ptr)->required_scope != EX(func)->common.scope) { | ||
| zend_type_error("Protected method %s cannot return private class %s", ZSTR_VAL(EX(func)->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name)); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm behavior of protected methods returning protected classes.
The code checks for a private class but does not explicitly handle the case of a protected method returning a protected class that belongs to another scope. If the same logic should control that scenario, consider adding a clear condition or comment explaining why it’s unnecessary.
🏁 Script executed:
#!/bin/bash
# Searches for calls or references to `required_scope_absolute` to confirm whether it covers all protected class checks.
rg 'required_scope_absolute'Length of output: 2041
Action: Add a clarifying comment regarding the scope-check logic in protected methods
Our investigation confirms that the flag required_scope_absolute is only set for private classes (as established in Zend/zend_compile.c). The check in Zend/zend_vm_def.h:
- Prevents a protected method from returning a private class by verifying that
Z_OBJCE_P(retval_ptr)->required_scope_absoluteis true and that the returned class’s required scope differs from the current function’s scope. - Implicitly allows protected classes to be returned since those classes never have
required_scope_absoluteset.
To improve code clarity, please add a comment here explaining that the absence of an explicit check for protected classes is intentional because only private classes are flagged via required_scope_absolute.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/classes/inner_classes/inheritance.phpt (1)
14-15:⚠️ Potential issueEmpty EXPECT section should be populated.
The test has an empty EXPECT section. PHP test files require expected output or error messages to validate test results. Without this, it's unclear what this test is checking for.
If this test is meant to validate that circular inheritance is properly detected or handled, then you should either:
- Include the expected error message
- Or document that no error should occur (empty output)
🧹 Nitpick comments (3)
tests/classes/inner_classes/simple_declaration_005.phpt (3)
34-34: Fix capitalization in expected error message.The expected error message uses lowercase "middle" but the actual class name is "Middle" with a capital M. This could cause the test to fail if the actual error message uses the correct capitalization.
-Fatal error: Declaration of Outer2::testSelf(): Outer2\middle must be compatible with Outer::testSelf(): Outer\middle in %s on line %d +Fatal error: Declaration of Outer2::testSelf(): Outer2\Middle must be compatible with Outer::testSelf(): Outer\Middle in %s on line %d
27-31: Remove redundant test call.Line 30 duplicates the call to
Outer2::testSelf()that's already made on line 29, which is unnecessary.var_dump(Outer::testSelf()); var_dump(Outer2::testParent()); var_dump(Outer2::testSelf()); -var_dump(Outer2::testSelf());
6-25: Document LSP violation with nested classes.This test demonstrates the Liskov Substitution Principle (LSP) violation with inner classes. When a subclass (
Outer2) overrides a method from its parent (Outer), the return type (Middle) refers to different classes in each context.Consider adding a comment at the top of the file explaining why this is occurring and how it relates to your PR objectives. For example:
// This test demonstrates the current behavior where inner classes with the same name // in parent and child classes are treated as different types, causing LSP violations. // The PR aims to address this by allowing inherited classes to recognize Outer\Middle // and Outer2\Middle as equivalent when referenced as simply "Middle".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Zend/zend_compile.c(20 hunks)tests/classes/inner_classes/inheritance.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_004.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_005.phpt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/classes/inner_classes/simple_declaration_004.phpt
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_compile.c (3)
Zend/zend_execute_API.c (2)
get_namespace_from_scope(1170-1176)get_scoped_name(1178-1189)Zend/zend_namespaces.h (2)
name(26-26)name(28-28)Zend/zend_execute.h (5)
name(37-37)name(51-51)name(52-52)name(53-53)name(422-422)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (21)
Zend/zend_compile.c (20)
41-41: Include for new namespace features looks correct.
The inclusion of"zend_namespaces.h"is necessary to access new namespace-handling functionality.
897-897: Allowing 'readonly', 'abstract', and 'final' for inner classes is consistent.
These changes ensure that the tokensT_READONLY,T_ABSTRACT, andT_FINALcan apply to inner classes. This supports the new nested class capabilities and complements the additional checks that follow.Also applies to: 902-902, 910-910
948-949: Adding a 'member' descriptor for inner classes.
This line extends the descriptive text for errors/reflection, clearly identifying “inner class” targets.
1057-1087: Restricting certain modifiers for inner classes.
This code disallows multiple PPP modifiers, forbidsstatic, and blocks attributing explicit public/private/protected sets on inner classes. This aligns with the intended limitations on nested classes.Please confirm that disallowing these modifiers on inner classes matches your desired language design and does not conflict with future extensibility.
1182-1201: Implementing scope-based class resolution.
The new functionsget_namespace_from_scope(),get_scoped_name(), andzend_resolve_class_in_scope()properly resolve nested class references, traversing lexical scopes. Memory handling viazend_string_release()appears correct, with no evident leaks.Also applies to: 1203-1235
1295-1300: Ensuring local scope resolution before namespace prefixing.
CheckingCG(active_class_entry)to see if an inner-scoped class is already known preserves the nested class lookup logic.
1862-1891: Recursive nested class name resolution.
zend_resolve_nested_class_name()handles multiple levels of nested classes by recursion. String operations and releases appear to be performed correctly.
1929-1933: Handling ZEND_AST_INNER_CLASS in constant expression resolution.
This branch correctly resolves nested class names by callingzend_resolve_nested_class_name(), extending the engine’s handling of class references within constant expressions.
3033-3034: Enforcing 'no static' rule on inner classes.
Disallowing thestaticmodifier for an inner class ensures consistent handling of nested class restrictions.Also applies to: 3038-3038
7460-7462: Special handling of inner classes in type compilation.
RecognizingZEND_AST_INNER_CLASSand resolving it viazend_resolve_nested_class_name()ensures the resultingzend_typeis properly constructed.
9127-9127: Introducing zend_compile_implements.
This function presumably manages interface implementation for both top-level and inner classes, aiding overall language consistency.
9187-9224: Deferred compilation of nested classes using inner_class_queue.
The code defers class declarations within an active class, storing them in a queue to be compiled after the parent is processed. This single-pass pattern looks sensible.
9251-9291: Renaming and flag assignment for nested classes.
Inner classes are renamed with theOuter\Innerformat, and flags such asfinal,abstract, andreadonlyare applied accordingly. This design properly scopes classes and respects the new modifiers.
9312-9313: Clearing scope requirements for anonymous classes.
Settingce->required_scopeandce->lexical_scopetoNULLcorrectly prevents any scope constraints on anonymous classes.
9355-9357: Compiling 'implements' for nested classes.
This addition ensures interfaces are accounted for even if the class is nested, reflecting consistent interface handling.
9373-9373: Scanning nested class declarations within a parent class.
zend_scan_nested_class_decl()ensures that all child classes inside the current class definition are recognized prior to compilation.
9403-9404: Jumping to compile_inner_classes after linking.
When the parent class is successfully resolved, the compilation of queued nested classes is triggered, preserving a logical ordering.
9412-9413: Reusing goto for nested class compilation branch.
Compiles inner classes once the current class has been linked in another code path, ensuring consistent post-linking behavior.
9482-9500: Processing the inner_class_queue.
After linking the parent class, the queued AST nodes for inner classes are compiled, then memory is freed. This completes the single-pass compilation pattern.
11770-11773: Deferring class declarations inside an active class.
When a new class is detected inside an existing class,zend_defer_class_decl()enqueues it, ensuring proper ordering for later compilation.tests/classes/inner_classes/simple_declaration_005.phpt (1)
1-35:❓ Verification inconclusive
Test contradicts PR objective about inheritance equivalence.
This test documents that
Outer\MiddleandOuter2\Middleare not treated as compatible types, resulting in a fatal error. However, one of the PR objectives is to "ensure that inheritance functions properly, allowing inherited classes to recognizeOuter\MiddleandOther\Middleas equivalent when usingMiddle."The current test validates that this equivalence does NOT work, which seems to contradict the PR goal. Either:
- The PR objective needs to be clarified, or
- This test should be updated to demonstrate the successful equivalence of these types after your implementation
If this test is intentionally showing the current behavior (pre-implementation), consider adding a comment clarifying that this is the behavior you're working to change.
🏁 Script executed:
#!/bin/bash # Look for other tests that might demonstrate successful inner class inheritance grep -r "testSelf" --include="*.phpt" tests/classes/inner_classes/Length of output: 1101
Attention: Inner Class Inheritance Mismatch
- The test in
tests/classes/inner_classes/simple_declaration_005.phptshows thatOuter\MiddleandOuter2\Middleare not recognized as equivalent, resulting in a fatal error when callingOuter2::testSelf().- One of the PR objectives is to have inherited inner classes recognized as equivalent (i.e., allowing
Middleto be used interchangeably regardless of whether it’s defined in the parent or the child class).- This test, as written, documents the current behavior where the types are incompatible.
- Action Required:
- Clarify the Objective: If the PR is intended to eventually allow such equivalence, please update the test with an explanatory comment stating that this fatal error is expected only in the current (pre-implementation) state.
- Or, Update Accordingly: If the desired outcome is to have the inherited inner classes be compatible, adjust the test to reflect the successful equivalence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Zend/zend_namespaces.c (2)
42-84: Consider consecutive slash handling and potential partial name collisions
- Consecutive backslashes (e.g.,
\\\\) will produce empty segments that might create unexpected “empty” namespaces.- The PR objective mentions recognizing certain namespace segments as equivalent (
Outer\Middlevs.Other\Middle), which the current hierarchical approach does not unify.Would you like a verification script to locate any test code or calls that may introduce consecutive slashes to confirm edge-case behavior?
117-123: Setglobal_namespacetoNULLafter destruction
To allow repeated calls without skipping cleanup, consider clearingglobal_namespaceafter destroying the hash table.Apply this diff to ensure a clean reset:
void zend_destroy_namespaces(void) { if (global_namespace == NULL) { return; } zend_hash_destroy(&namespaces); + global_namespace = NULL; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Zend/zend_namespaces.c(1 hunks)Zend/zend_opcode.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Zend/zend_opcode.c
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (5)
Zend/zend_namespaces.c (5)
1-19: License header is well-structured
No concerns with the licensing block.
20-28: Confirm thread-safety requirements
Currently, the global variablesglobal_namespaceandnamespacesare defined without synchronization primitives. This is acceptable if the Zend Engine enforces a single-threaded model or if usage is implicitly protected elsewhere. Otherwise, concurrent reads/writes could lead to data races. Please verify whether concurrency is a requirement.
30-40: Good creation logic
Memory allocation and class initialization appear correct and consistent with typical Zend Engine patterns. No issues found.
86-107: Ensure initialization is concurrency-safe
If multiple threads callzend_resolve_namespacebeforeglobal_namespaceis initialized, undefined behavior could occur due to simultaneous writes. Consider adding a lock or verifying that this is only called under single-threaded conditions.
109-115: Straightforward lookup
This function is concise and aligns with typical usage, returning existing namespaces without creating them. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Zend/zend_namespaces.c (1)
115-122:⚠️ Potential issueMemory cleanup is incomplete.
zend_hash_destroy(&namespaces)will free hashed entries, butglobal_namespaceis also allocated viamalloc(or ideallyemalloc). Either:
- Use a class Dtor that handles
efree(ns)internally if you continue withemalloc.- Manually free
global_namespaceitself here if you preserve rawmalloc/freeusage.Ensure consistency between allocation and deallocation to avoid leaks or corruption.
Example fix for the final cleanup:
void zend_destroy_namespaces(void) { if (global_namespace == NULL) { return; } zend_hash_destroy(&namespaces); -} + efree(global_namespace); + global_namespace = NULL; +}
🧹 Nitpick comments (1)
Zend/zend_namespaces.c (1)
41-82: Streamline namespace-part parsing.The splitting logic in the
insert_namespacefunction is correct, but consider:
- Using a for-loop over the string length for clarity.
- Handling leading/trailing backslashes or empty segments as desired (if relevant to your code style).
This is a stylistic choice; the current approach works but could be simplified for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Zend/zend_namespaces.c(1 hunks)tests/classes/inner_classes/reflection_001.phpt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/classes/inner_classes/reflection_001.phpt
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.c (6)
Zend/zend_execute_API.c (4)
zend_class_entry(710-722)zend_class_entry(1226-1371)zend_class_entry(1374-1377)zend_class_entry(1380-1395)Zend/zend_inheritance.c (2)
zend_class_entry(3520-3792)zend_class_entry(3887-3969)Zend/zend_compile.c (1)
zend_class_entry(1406-1446)Zend/zend_compile.h (2)
zend_initialize_class_data(983-983)name(988-988)Zend/zend_namespaces.h (3)
name(25-25)name(27-27)zend_destroy_namespaces(26-26)Zend/zend_execute.h (5)
name(37-37)name(51-51)name(52-52)name(53-53)name(422-422)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: FREEBSD
🔇 Additional comments (4)
Zend/zend_namespaces.c (4)
1-19: License header looks good.All license text and attributions appear in order.
20-29: Global static variables are acceptable here.Given that the Zend Engine is typically single-threaded under normal usage, using global static variables (
global_namespaceandnamespaces) is fine. No immediate concurrency issues are detected for this usage.
84-105: Namespace resolution logic is clear.The fallback to the global namespace on empty input and the lowercase lookup are consistent with case-insensitive namespace behavior. The overall approach aligns well with typical PHP/Zend naming conventions.
107-113: Lookup function is straightforward and correct.Using
zend_lookup_namespaceto retrieve the class entry by lowercase name is consistent with the resolution logic. No immediate issues found.
this adds all the initial opcode handling and special parts -- or the basics anyway.
This reverts commit 26b7eb7.
withinboredom
force-pushed
the
rfc/inner-class3
branch
from
99269a2 to
4ac9b82
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Zend/zend_API.c (1)
1819-1841:⚠️ Potential issuePotential NULL dereference in
scope->lexical_scopeis still presentThe condition on line 1835 calls
instanceof_function(scope->lexical_scope, class_type->required_scope)without checking ifscope->lexical_scopeis NULL. This could lead to a segmentation fault if the current execution scope doesn't have a lexical scope.A previous review flagged this issue, but the fix doesn't appear to be implemented in this code. The check should include an explicit NULL check before dereferencing
scope->lexical_scope.Apply this diff to fix the potential NULL dereference:
- } else if (!instanceof_function(scope, class_type->required_scope) && !instanceof_function(scope->lexical_scope, class_type->required_scope)) { + } else if (!instanceof_function(scope, class_type->required_scope) + && (scope->lexical_scope == NULL + || !instanceof_function(scope->lexical_scope, class_type->required_scope))) {Zend/zend_language_parser.y (1)
977-978: 🛠️ Refactor suggestionInconsistent handling of modifiers between top-level and inner classes
While your implementation works, there's an inconsistency in how modifiers are applied:
- For top-level classes (lines 599-604), modifiers are passed directly to the AST creation
- For inner classes, the AST is created with flags=0, then modifiers are applied afterward via attr
This inconsistency could make the code harder to maintain. Consider using a consistent approach for both cases.
- inner_class_modifiers inner_class_statement - { $$ = $2; $$->attr = $1; } + inner_class_modifiers T_CLASS { $<num>$ = CG(zend_lineno); } + T_STRING extends_from implements_list backup_doc_comment '{' class_statement_list '}' + { $$ = zend_ast_create_decl(ZEND_AST_CLASS, $1, $<num>3, $7, zend_ast_get_str($4), $5, $6, $9, NULL, NULL); }
🧹 Nitpick comments (6)
Zend/zend_vm_def.h (1)
4431-4439: Ensure consistency in error reporting for public methods returning restricted classes.This block correctly prevents a public method from returning either a private or protected class, but consider clarifying the user-facing error messages if both scenarios (private vs. protected) share similar root causes.
Zend/zend_compile.c (2)
9403-9403: Control flow with goto.
Jumping tocompile_inner_classesviagotois typical in Zend code for large sections handling. While it’s idiomatic within this codebase, consider re-checking any side paths that might skip essential cleanup if new logic is added later.
9412-9412: Another goto usage.
Similarly, thisgoto compile_inner_classes;works in tandem with the prior logic. Just ensure future modifications maintain correct control flow.Zend/zend_execute_API.c (3)
1170-1176: Clean implementation of namespace retrieval from class scope.This function traverses a class's lexical scope hierarchy until finding a namespace class, ensuring proper namespace resolution for inner classes.
Consider adding a documentation comment explaining the purpose of this function and its relationship to inner class handling.
1178-1189: Well-implemented scoped name processing function.This function handles removing the namespace prefix from a class name when appropriate, which is necessary for proper resolution of inner classes.
Two minor suggestions:
- Add a documentation comment explaining what this function does
- Consider adding a NULL check for the
nameparameter before using it
1191-1224: Solid implementation of class lookup within lexical scope.This function effectively traverses the lexical scope hierarchy to find classes, building fully-qualified names at each level. The memory management is handled correctly, with all allocated strings being properly released.
A couple of suggestions for improvement:
- Add a documentation comment explaining the purpose and logic of this function
- Consider adding a NULL check for the
nameparameter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (74)
Zend/Optimizer/dfa_pass.c(1 hunks)Zend/tests/errmsg/errmsg_027.phpt(1 hunks)Zend/zend.c(2 hunks)Zend/zend.h(1 hunks)Zend/zend_API.c(1 hunks)Zend/zend_ast.h(1 hunks)Zend/zend_compile.c(20 hunks)Zend/zend_compile.h(3 hunks)Zend/zend_execute.c(2 hunks)Zend/zend_execute.h(1 hunks)Zend/zend_execute_API.c(3 hunks)Zend/zend_inheritance.c(2 hunks)Zend/zend_language_parser.y(6 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_namespaces.h(1 hunks)Zend/zend_object_handlers.c(7 hunks)Zend/zend_opcode.c(1 hunks)Zend/zend_vm_def.h(1 hunks)Zend/zend_vm_execute.h(5 hunks)configure.ac(1 hunks)ext/opcache/zend_persist.c(2 hunks)ext/reflection/php_reflection.c(1 hunks)ext/reflection/php_reflection.stub.php(1 hunks)ext/reflection/php_reflection_arginfo.h(4 hunks)ext/reflection/tests/ReflectionClass_toString_001.phpt(2 hunks)ext/reflection/tests/bug74454.phpt(1 hunks)tests/classes/.idea/.gitignore(1 hunks)tests/classes/.idea/classes.iml(1 hunks)tests/classes/.idea/deployment.xml(1 hunks)tests/classes/.idea/misc.xml(1 hunks)tests/classes/.idea/modules.xml(1 hunks)tests/classes/.idea/php.xml(1 hunks)tests/classes/.idea/vcs.xml(1 hunks)tests/classes/inner_classes/access_modifiers_001.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_002.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_003.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_004.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_005.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_006.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_007.phpt(1 hunks)tests/classes/inner_classes/autoload_001.phpt(1 hunks)tests/classes/inner_classes/autoload_002.phpt(1 hunks)tests/classes/inner_classes/enum_usage.phpt(1 hunks)tests/classes/inner_classes/errors_001.phpt(1 hunks)tests/classes/inner_classes/errors_002.phpt(1 hunks)tests/classes/inner_classes/inheritance.phpt(1 hunks)tests/classes/inner_classes/inner_classes.inc(1 hunks)tests/classes/inner_classes/interface_usage.phpt(1 hunks)tests/classes/inner_classes/readonly_001.phpt(1 hunks)tests/classes/inner_classes/reflection_001.phpt(1 hunks)tests/classes/inner_classes/return_types_001.phpt(1 hunks)tests/classes/inner_classes/return_types_002.phpt(1 hunks)tests/classes/inner_classes/return_types_003.phpt(1 hunks)tests/classes/inner_classes/return_types_004.phpt(1 hunks)tests/classes/inner_classes/return_types_005.phpt(1 hunks)tests/classes/inner_classes/return_types_006.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_001.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_002.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_003.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_004.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_005.phpt(1 hunks)tests/classes/inner_classes/static_variables.phpt(1 hunks)tests/classes/inner_classes/trait_usage.phpt(1 hunks)tests/classes/inner_classes/visibility_001.phpt(1 hunks)tests/classes/inner_classes/visibility_002.phpt(1 hunks)tests/classes/inner_classes/visibility_003.phpt(1 hunks)tests/classes/inner_classes/visibility_004.phpt(1 hunks)tests/classes/inner_classes/visibility_005.phpt(1 hunks)tests/classes/inner_classes/visibility_006.phpt(1 hunks)tests/classes/inner_classes/visibility_007.phpt(1 hunks)tests/classes/inner_classes/visibility_008.phpt(1 hunks)tests/classes/inner_classes/visibility_009.phpt(1 hunks)tests/classes/inner_classes/visibility_010.phpt(1 hunks)win32/build/config.w32(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (63)
- tests/classes/.idea/.gitignore
- Zend/tests/errmsg/errmsg_027.phpt
- tests/classes/inner_classes/errors_001.phpt
- tests/classes/.idea/misc.xml
- tests/classes/.idea/php.xml
- tests/classes/.idea/modules.xml
- tests/classes/inner_classes/access_modifiers_004.phpt
- tests/classes/inner_classes/return_types_001.phpt
- tests/classes/inner_classes/access_modifiers_005.phpt
- tests/classes/.idea/deployment.xml
- tests/classes/.idea/vcs.xml
- tests/classes/inner_classes/access_modifiers_001.phpt
- tests/classes/inner_classes/access_modifiers_006.phpt
- Zend/zend_vm_execute.h
- tests/classes/.idea/classes.iml
- configure.ac
- tests/classes/inner_classes/return_types_003.phpt
- tests/classes/inner_classes/access_modifiers_003.phpt
- tests/classes/inner_classes/reflection_001.phpt
- ext/reflection/tests/bug74454.phpt
- tests/classes/inner_classes/return_types_005.phpt
- tests/classes/inner_classes/visibility_008.phpt
- Zend/zend_ast.h
- tests/classes/inner_classes/visibility_001.phpt
- tests/classes/inner_classes/return_types_006.phpt
- tests/classes/inner_classes/inheritance.phpt
- tests/classes/inner_classes/interface_usage.phpt
- tests/classes/inner_classes/trait_usage.phpt
- Zend/zend_execute.h
- tests/classes/inner_classes/return_types_004.phpt
- tests/classes/inner_classes/access_modifiers_002.phpt
- tests/classes/inner_classes/readonly_001.phpt
- tests/classes/inner_classes/visibility_004.phpt
- tests/classes/inner_classes/simple_declaration_002.phpt
- tests/classes/inner_classes/inner_classes.inc
- tests/classes/inner_classes/access_modifiers_007.phpt
- ext/reflection/tests/ReflectionClass_toString_001.phpt
- tests/classes/inner_classes/autoload_001.phpt
- Zend/zend_namespaces.h
- tests/classes/inner_classes/simple_declaration_001.phpt
- tests/classes/inner_classes/simple_declaration_003.phpt
- Zend/zend.h
- tests/classes/inner_classes/static_variables.phpt
- tests/classes/inner_classes/visibility_009.phpt
- Zend/Optimizer/dfa_pass.c
- tests/classes/inner_classes/errors_002.phpt
- win32/build/config.w32
- tests/classes/inner_classes/visibility_003.phpt
- tests/classes/inner_classes/visibility_006.phpt
- Zend/zend.c
- ext/reflection/php_reflection.stub.php
- tests/classes/inner_classes/enum_usage.phpt
- tests/classes/inner_classes/visibility_002.phpt
- tests/classes/inner_classes/return_types_002.phpt
- tests/classes/inner_classes/simple_declaration_005.phpt
- tests/classes/inner_classes/visibility_007.phpt
- ext/reflection/php_reflection_arginfo.h
- tests/classes/inner_classes/autoload_002.phpt
- tests/classes/inner_classes/visibility_005.phpt
- Zend/zend_namespaces.c
- tests/classes/inner_classes/simple_declaration_004.phpt
- tests/classes/inner_classes/visibility_010.phpt
- Zend/zend_inheritance.c
🧰 Additional context used
🧬 Code Definitions (2)
Zend/zend_execute.c (5)
Zend/zend_compile.c (8)
bool(309-323)bool(527-530)bool(2112-2122)bool(2125-2136)bool(2499-2524)bool(9938-9958)bool(9960-10022)bool(10037-10048)Zend/zend_object_handlers.c (3)
bool(259-270)bool(971-976)bool(1591-1615)Zend/zend_ast.h (2)
bool(352-354)bool(356-358)Zend/zend_object_handlers.h (2)
ce(249-249)ce(313-313)Zend/zend_execute.h (1)
zend_verify_property_type_error(529-529)
Zend/zend_compile.c (3)
Zend/zend_execute_API.c (2)
get_namespace_from_scope(1170-1176)get_scoped_name(1178-1189)Zend/zend_namespaces.h (2)
name(25-25)name(27-27)Zend/zend_namespaces.c (1)
zend_resolve_namespace(84-106)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (42)
ext/opcache/zend_persist.c (2)
1124-1143: Implementation of required scope handling for opcacheThis new function correctly handles the translation of
required_scopeandlexical_scopepointers when persisting classes to shared memory. It ensures that inner class relationships are properly maintained when classes are stored in opcache.
1318-1318: Properly updating scope relationships during class persistenceThe call to
zend_update_required_scope(ce)after updating parent classes is strategically placed to ensure that all class relationships are properly translated when persisting to shared memory.Zend/zend_compile.h (3)
897-897: Support for inner class modifiersAddition of
ZEND_MODIFIER_TARGET_INNER_CLASSto the modifier target enum enables proper handling of access modifiers for inner classes, which is essential for implementing the RFC requirements.
1024-1024: Implementation of outer class fetchingThe new
ZEND_FETCH_CLASS_OUTERconstant enables the engine to resolve and fetch outer classes, which is a fundamental requirement for inner class functionality. This value is properly placed within the existing class fetch constants.
1065-1065: Added namespace class type supportThe
ZEND_NAMESPACE_CLASSconstant adds support for properly identifying classes within namespaces, ensuring that namespace structure works correctly with opcache as specified in the PR objectives.Zend/zend_opcode.c (2)
433-436: Memory management for required_scope looks good.The addition of reference count decrement and nullification for
ce->required_scopeproperly handles the memory management of this new property. This ensures there are no memory leaks when destroying class entries that have required scopes, which is crucial for the inner class implementation.
439-439: ZEND_NAMESPACE_CLASS case added correctly.Adding
ZEND_NAMESPACE_CLASSas a case that shares the same destruction logic asZEND_INTERNAL_CLASSis appropriate. This ensures proper destruction of namespace classes, supporting the PR's objective of ensuring namespace structure operates correctly with opcache.Zend/zend_vm_def.h (1)
4440-4445: Add a clarifying comment for protected methods returning protected classes.Currently, there's a block checking that a protected method cannot return a private class from another scope, but there's no explicit check for returning a protected class from an unrelated scope. This may be intentional because only privately scoped classes set
required_scope_absolute, while protected classes do not trigger this condition. However, adding a short comment here would help clarify that logic.Below is a script to verify whether returning a protected class from a different scope is handled elsewhere or intentionally allowed:
#!/bin/bash # Looking for references to `ZEND_ACC_PROTECTED` and # `required_scope` to see if protected classes are checked in other places. rg -A 5 'ZEND_ACC_PROTECTED' rg -A 5 '->required_scope'ext/reflection/php_reflection.c (4)
4079-4089: New method to check for inner classesThis new method provides a way to determine if a class is an inner class by checking if it has a lexical scope that isn't a namespace. This supports the inner class implementation, making it possible to introspect class relationships through reflection.
The implementation correctly checks both that:
- The class has a lexical scope (parent class context)
- The lexical scope is not a namespace (ZEND_NAMESPACE_CLASS)
4092-4101: Added visibility check for private classesThis method implements class-level visibility checking for the private access modifier. It correctly identifies a private class when:
required_scopeis set (indicating visibility restriction)required_scope_absoluteis also set (making it private rather than protected)This aligns with the PR objective to properly support the namespace structure and inheritance for inner classes.
4104-4113: Added visibility check for protected classesThis method identifies protected classes by checking if
required_scopeis set (visibility restriction exists) butrequired_scope_absoluteis not set (distinguishing it from private).The implementation follows the same pattern as the other visibility methods, creating a consistent API.
4116-4122: Added visibility check for public classesThis method provides a simple way to determine if a class is public, correctly identifying public classes as those that don't have a
required_scope(no visibility restrictions).Implementation is clean and consistent with the other visibility checking methods.
Zend/zend_compile.c (18)
41-41: Include usage check.
Including"zend_namespaces.h"here seems aligned with the need for namespace resolution. Please ensure that all new references to namespace-related functions (e.g.,zend_resolve_namespace) are properly guarded to avoid unintended side effects when namespaces are unavailable or not in use.
897-913: Modifier checks for inner classes look consistent.
AddingZEND_MODIFIER_TARGET_INNER_CLASSto these checks ensures that modifiers likereadonly,abstract, andfinalcan apply to inner classes. This is a logical extension of the existing modifier checks. No immediate issues detected.
948-950: Enhanced error reporting.
Identifying the modifier target as an "inner class" for error messages helps provide more descriptive feedback to the developer. This change appears correct and consistent with the new modifier target.
1057-1087: Explicit constraints on inner classes.
By disallowing multiple access modifiers, static, or any form of public/protected/private sets on inner classes, you are strictly confining their usage. If this strictness is intentional, it’s fine; otherwise, consider whether partial support for “public” or “protected” inner classes would be beneficial.Do you want to allow any form of controlled visibility for inner classes in future iterations?
1182-1235: Namespace resolution logic.
The new utility methods (get_namespace_from_scope,get_scoped_name,zend_resolve_class_in_scope) properly walk lexical scopes and strip namespace prefixes. The memory-release calls look correct. Keep an eye out for potential edge cases with top-level or unnamed namespaces.
1295-1300: Conditionally resolving classes within active scope.
Callingzend_resolve_class_in_scopewhenCG(active_class_entry)is set makes sense to maintain consistency with nested contexts. The logic here appears coherent.
1860-1897: Recursive name resolution for nested classes.
zend_resolve_nested_class_nameand the associated logic inzend_try_compile_const_expr_resolve_class_nameproperly handle multiple nesting levels. This approach guards against partial or incomplete name references.
1929-1933: Inner-class constant expression resolution.
This snippet ensures that compile-time constant expression resolution gracefully handles inner classes by invokingzend_resolve_nested_class_name. No issues found.
3033-3034: Static modifier restriction on inner classes.
Raising a compile error when attemptingstaticon an inner class aligns with earlier logic that disallows static for inner classes. This maintains consistency.
7460-7462: Inner class type resolution.
Auto-detecting nested classes inzend_compile_single_typenameallows the compiler to differentiate between regular class references and inner class references. This is a clear approach for type resolution.
9127-9127: Implements directive for inner classes.
Keepingzend_compile_implementsaccessible here suggests inner classes may eventually implement interfaces. The declaration signature change is straightforward.
9187-9202: Deferred compilation queue for inner classes.
Storing inner classes ininner_class_queuepostpones their compilation until the outer class is fully compiled. This approach prevents incomplete lexical contexts. Looks well-structured.
9251-9292: Name collision considerations.
Renaming inner classes to<Outer>\Inneris a neat approach for referencing. Be mindful of potential naming collisions if outer classes share the same name in different namespaces. The handling ofrequired_scopeandlexical_scopeis logical.Would you like to scan the codebase to confirm no collisions occur in common naming patterns?
9312-9313: Null scope for anonymous classes.
Settingce->required_scopeandce->lexical_scopeto NULL for effectively anonymous classes is consistent with how unnamed classes function in standard PHP.
9355-9357: Interface implementation check.
Leveragingzend_compile_implementsensures that any interfaces declared on an inner class are properly processed. Straightforward extension of existing logic.
9373-9373: Initiating scan for nested classes.
zend_scan_nested_class_decl(stmt_ast);looks like a pre-compilation pass to collect references. No issues spotted.
9482-9500: Compiling deferred inner classes.
Draininginner_class_queueensures all inner classes are resolved after the outer class is linked. Thread-safety generally isn’t a concern here since compilation is single-threaded, but keep that assumption in mind if concurrency is ever introduced.
11790-11793: Deferred vs. direct class compilation.
Defer logic is triggered ifCG(active_class_entry)is set, preventing illegal nested declarations. This is consistent with earlier constraints. Implementation appears to meet the design intent.Zend/zend_execute_API.c (2)
23-23: Include for namespace handling added properly.The new include of
zend_namespaces.his appropriate for the namespace-related functionality being introduced.
1242-1264: Good integration of scoped lookup into the class lookup system.The modifications to
zend_lookup_class_exproperly integrate the new scoped lookup functionality for both compilation and execution contexts. The cache is correctly updated when a class is found.Zend/zend_object_handlers.c (5)
384-409: Excellent addition of lexical scope handling for property access!This change enables inner classes to access private/protected properties of their outer classes by traversing up the lexical scope chain when a direct visibility check fails. The implementation is clean and maintains the original error handling flow.
823-847: Good implementation of lexical scope traversal for method access.This change properly extends the method access checks to consider the lexical scope chain for inner classes. The code correctly preserves the original scope for error reporting purposes while enabling inner class methods to access private or protected methods of their containing classes.
905-917: Well-implemented lexical scope traversal for static method access.The implementation correctly handles static method access checks for inner classes by traversing up the lexical scope chain. Good job preserving the original scope for proper error reporting.
994-2002: Effective implementation of lexical scope handling for static property access.This change appropriately extends the static property access checks to consider the lexical scope chain, allowing inner classes to access static properties of their outer classes while maintaining proper visibility rules.
2083-2091: Appropriate extension of lexical scope handling to constructors.This change ensures that inner classes can properly access constructors of their outer classes by traversing the lexical scope chain. The implementation is consistent with the other lexical scope handling changes throughout the file.
Zend/zend_execute.c (1)
1082-1085: Confirm usage ofinfo->flagsfor property visibility.Here, the code passes
info->flagstozend_check_class_visibility()as the current visibility. Ensure thatinfo->flagsindeed reflects the property’s access level. If you want only the public/protected/private bits, consider masking withZEND_ACC_PPP_MASK. Otherwise, additional flags might produce incorrect checks.Would you like me to generate a script to confirm where these flags are set in the codebase?
Zend/zend_language_parser.y (4)
954-957: Consider consolidating modifier flags withinzend_ast_create_declCurrently, the class declaration is created with hardcoded flags set to 0. If you anticipate needing to support various flags on inner classes, you might consider injecting the computed flags at creation time, as is done with top-level classes, to avoid potential confusion down the road.
631-637: LGTM: Inner class modifiers implementation looks goodThe implementation of inner class modifiers follows the same pattern as other modifier rules like
method_modifiersandclass_const_modifiers. It correctly implements the default visibility (public) when none is specified, and properly validates modifier combinations.
288-288: LGTM: Type declaration for new grammar rulesThe type declaration for
inner_class_statementis properly added to support the new grammar rules.
291-291: LGTM: Added inner_class_modifiers to numeric typesThis addition properly declares the new rule for inner class modifiers as a numeric type, consistent with other modifier types.
| static zend_always_inline bool zend_check_class_visibility(const zend_class_entry *ce, const zend_property_info *info, uint32_t current_visibility) { | ||
| // a public class is always visible | ||
| if (!ce->required_scope) { | ||
| return 1; | ||
| } | ||
|
|
||
| // a protected class is visible if it is a subclass of the lexical scope and the current visibility is protected or private | ||
| if (!ce->required_scope_absolute) { | ||
| if (current_visibility & ZEND_ACC_PUBLIC) { | ||
| zend_type_error("Cannot assign protected %s to higher visibile property %s::%s", | ||
| ZSTR_VAL(ce->name), | ||
| ZSTR_VAL(info->ce->name), | ||
| zend_get_unmangled_property_name(info->name)); | ||
| return 0; | ||
| } | ||
|
|
||
| return 1; | ||
| } | ||
|
|
||
| // a private class is visible if it is the same class as the lexical scope and the current visibility is private | ||
| if (ce->required_scope_absolute && current_visibility & ZEND_ACC_PRIVATE) { | ||
| return 1; | ||
| } | ||
|
|
||
| zend_type_error("Cannot assign private %s to higher visibile property %s::%s", | ||
| ZSTR_VAL(ce->name), | ||
| ZSTR_VAL(info->ce->name), | ||
| zend_get_unmangled_property_name(info->name)); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Fix logic mismatch and correct spelling in error messages.
The function’s doc comment indicates that protected classes require checking if the assigned class is indeed a subclass of the current scope, but the implementation does not perform that check. This discrepancy can cause incorrect behavior. Additionally, there's a spelling error in the error messages: “visibile” should be “visible.” Below is a patch that addresses the spelling issue and clarifies the error message text:
static zend_always_inline bool zend_check_class_visibility(const zend_class_entry *ce, const zend_property_info *info, uint32_t current_visibility) {
// a public class is always visible
if (!ce->required_scope) {
return 1;
}
// a protected class is visible if it is a subclass of the lexical scope and the current visibility is protected or private
if (!ce->required_scope_absolute) {
if (current_visibility & ZEND_ACC_PUBLIC) {
- zend_type_error("Cannot assign protected %s to higher visibile property %s::%s",
+ zend_type_error("Cannot assign protected %s to a property with higher visibility %s::%s",
ZSTR_VAL(ce->name),
ZSTR_VAL(info->ce->name),
zend_get_unmangled_property_name(info->name));
return 0;
}
return 1;
}
// a private class is visible if it is the same class as the lexical scope and the current visibility is private
if (ce->required_scope_absolute && current_visibility & ZEND_ACC_PRIVATE) {
return 1;
}
- zend_type_error("Cannot assign private %s to higher visibile property %s::%s",
+ zend_type_error("Cannot assign private %s to a property with higher visibility %s::%s",
ZSTR_VAL(ce->name),
ZSTR_VAL(info->ce->name),
zend_get_unmangled_property_name(info->name));
return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static zend_always_inline bool zend_check_class_visibility(const zend_class_entry *ce, const zend_property_info *info, uint32_t current_visibility) { | |
| // a public class is always visible | |
| if (!ce->required_scope) { | |
| return 1; | |
| } | |
| // a protected class is visible if it is a subclass of the lexical scope and the current visibility is protected or private | |
| if (!ce->required_scope_absolute) { | |
| if (current_visibility & ZEND_ACC_PUBLIC) { | |
| zend_type_error("Cannot assign protected %s to higher visibile property %s::%s", | |
| ZSTR_VAL(ce->name), | |
| ZSTR_VAL(info->ce->name), | |
| zend_get_unmangled_property_name(info->name)); | |
| return 0; | |
| } | |
| return 1; | |
| } | |
| // a private class is visible if it is the same class as the lexical scope and the current visibility is private | |
| if (ce->required_scope_absolute && current_visibility & ZEND_ACC_PRIVATE) { | |
| return 1; | |
| } | |
| zend_type_error("Cannot assign private %s to higher visibile property %s::%s", | |
| ZSTR_VAL(ce->name), | |
| ZSTR_VAL(info->ce->name), | |
| zend_get_unmangled_property_name(info->name)); | |
| return 0; | |
| } | |
| static zend_always_inline bool zend_check_class_visibility(const zend_class_entry *ce, const zend_property_info *info, uint32_t current_visibility) { | |
| // a public class is always visible | |
| if (!ce->required_scope) { | |
| return 1; | |
| } | |
| // a protected class is visible if it is a subclass of the lexical scope and the current visibility is protected or private | |
| if (!ce->required_scope_absolute) { | |
| if (current_visibility & ZEND_ACC_PUBLIC) { | |
| zend_type_error("Cannot assign protected %s to a property with higher visibility %s::%s", | |
| ZSTR_VAL(ce->name), | |
| ZSTR_VAL(info->ce->name), | |
| zend_get_unmangled_property_name(info->name)); | |
| return 0; | |
| } | |
| return 1; | |
| } | |
| // a private class is visible if it is the same class as the lexical scope and the current visibility is private | |
| if (ce->required_scope_absolute && current_visibility & ZEND_ACC_PRIVATE) { | |
| return 1; | |
| } | |
| zend_type_error("Cannot assign private %s to a property with higher visibility %s::%s", | |
| ZSTR_VAL(ce->name), | |
| ZSTR_VAL(info->ce->name), | |
| zend_get_unmangled_property_name(info->name)); | |
| return 0; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Zend/zend_namespaces.c (3)
84-106: Add error handling for hash initialization.The function initializes a hash table but doesn't check for allocation failures.
if (global_namespace == NULL) { global_namespace = create_namespace(zend_string_copy(zend_empty_string)); global_namespace->lexical_scope = NULL; - zend_hash_init(&namespaces, 8, NULL, ZEND_CLASS_DTOR, 1); + if (zend_hash_init(&namespaces, 8, NULL, ZEND_CLASS_DTOR, 1) == FAILURE) { + zend_error(E_ERROR, "Failed to initialize namespace hash table"); + return NULL; + } zend_hash_add_ptr(&namespaces, zend_empty_string, global_namespace); }
116-122: Improve cleanup in zend_destroy_namespaces.The function destroys the hash table but doesn't reset the
global_namespacepointer, which could lead to use-after-free issues if the function is called multiple times.void zend_destroy_namespaces(void) { if (global_namespace == NULL) { return; } zend_hash_destroy(&namespaces); + global_namespace = NULL; // Reset the pointer after cleanup }
108-114: Consider adding null check for consistency.For consistency with other functions, consider adding a null check for the
nameparameter inzend_lookup_namespace.zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL || ZSTR_LEN(name) == 0) { + return global_namespace; + } zend_string *lc_name = zend_string_tolower(name); zend_class_entry *ns = zend_hash_find_ptr(&namespaces, lc_name); zend_string_release(lc_name); return ns; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_namespaces.c(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.c (6)
Zend/zend_execute_API.c (4)
zend_class_entry(710-722)zend_class_entry(1226-1371)zend_class_entry(1374-1377)zend_class_entry(1380-1395)Zend/zend_inheritance.c (2)
zend_class_entry(3520-3792)zend_class_entry(3887-3969)Zend/zend_compile.c (1)
zend_class_entry(1406-1446)Zend/zend_compile.h (2)
zend_initialize_class_data(983-983)name(988-988)Zend/zend_namespaces.h (3)
name(25-25)name(27-27)zend_destroy_namespaces(26-26)Zend/zend_execute.h (5)
name(37-37)name(51-51)name(52-52)name(53-53)name(422-422)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: FREEBSD
🔇 Additional comments (3)
Zend/zend_namespaces.c (3)
30-39: Replace malloc with emalloc to use Zend memory manager.Using
mallocinstead of Zend's memory management functions can lead to inconsistent memory handling. The Zend Engine expects memory to be allocated with its internal memory manager to properly handle cleanup during request shutdown.Apply this diff to switch to Zend's memory manager:
-static zend_class_entry *create_namespace(zend_string *interned_name) { - zend_class_entry *ns = malloc(sizeof(zend_class_entry)); +static zend_class_entry *create_namespace(zend_string *interned_name) { + zend_class_entry *ns = emalloc(sizeof(zend_class_entry)); zend_initialize_class_data(ns, 1); ns->type = ZEND_NAMESPACE_CLASS; ns->ce_flags |= ZEND_ACC_UNINSTANTIABLE; ns->name = interned_name; return ns; }
1-18: The file header looks good.The license and copyright information is properly included, following the standard format for Zend Engine source files.
41-82:⚠️ Potential issuePotential boundary issue and potential memory leak.
The loop condition
pos <= endmight read one byte past the end of the string. Additionally,needleis created withzend_string_init_internedbut might not be properly released ifzend_hash_find_ptrsucceeds.Consider these improvements:
- Fix the boundary check:
- while (pos <= end) { + while (pos < end) {
- Manage memory for
needleconsistently:zend_string *needle = zend_string_init_interned(ZSTR_VAL(current_ns.s), ZSTR_LEN(current_ns.s), 1); ns = zend_hash_find_ptr(&namespaces, needle); zend_string_release(part); if (!ns) { ns = create_namespace(needle); ns->parent = parent_ns; zend_hash_add_ptr(&namespaces, needle, ns); + } else { + zend_string_release(needle); }Likely an incorrect or invalid review comment.
Zend/zend_namespaces.c
Outdated
| while (pos <= end) { | ||
| if (pos == end || *pos == '\\') { | ||
| len = pos - start; | ||
| part = zend_string_init(start, len, 0); | ||
|
|
||
| if (current_ns.s) { | ||
| smart_str_appendc(¤t_ns, '\\'); | ||
| } | ||
| smart_str_appendl(¤t_ns, ZSTR_VAL(part), ZSTR_LEN(part)); | ||
| smart_str_0(¤t_ns); | ||
|
|
||
| zend_string *needle = zend_string_init_interned(ZSTR_VAL(current_ns.s), ZSTR_LEN(current_ns.s), 1); | ||
| ns = zend_hash_find_ptr(&namespaces, needle); | ||
|
|
||
| zend_string_release(part); | ||
| if (!ns) { | ||
| ns = create_namespace(needle); | ||
| ns->parent = parent_ns; | ||
| zend_hash_add_ptr(&namespaces, needle, ns); | ||
| } | ||
|
|
||
| parent_ns = ns; | ||
| start = pos + 1; | ||
| } | ||
| pos++; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a cleaner namespace parsing approach.
The current parsing logic is complex and prone to errors. Consider simplifying it by using PHP's existing namespace parsing functions or a more structured approach.
A more robust approach would be to use php_strtok_r or similar functions to split the namespace string by backslashes, which would make the code more readable and less error-prone. Here's a conceptual example (not a full implementation):
char *saveptr;
char *token;
zend_string *ns_copy = zend_string_dup(name, 0);
smart_str current_ns = {0};
token = php_strtok_r(ZSTR_VAL(ns_copy), "\\", &saveptr);
while (token != NULL) {
if (current_ns.s) {
smart_str_appendc(¤t_ns, '\\');
}
smart_str_appends(¤t_ns, token);
smart_str_0(¤t_ns);
// Create or find namespace entry
// ...
token = php_strtok_r(NULL, "\\", &saveptr);
}
zend_string_release(ns_copy);Alternatively, you could leverage existing functionality in the Zend engine for parsing namespace names.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Zend/zend_namespaces.c (1)
30-39:⚠️ Potential issueUse Zend memory management functions instead of
malloc.Allocating the
zend_class_entryviamalloccan lead to mismatched memory release if the engine expectsefree()for allocated memory. This could result in memory leaks or corruption.Apply this diff to switch to Zend's memory manager:
static zend_class_entry *create_namespace(zend_string *interned_name) { - zend_class_entry *ns = malloc(sizeof(zend_class_entry)); + zend_class_entry *ns = emalloc(sizeof(zend_class_entry)); zend_initialize_class_data(ns, 1); ns->type = ZEND_NAMESPACE_CLASS; ns->ce_flags |= ZEND_ACC_UNINSTANTIABLE; ns->name = interned_name; return ns; }
🧹 Nitpick comments (2)
Zend/zend_namespaces.c (2)
1-19: Add function documentation comments.The file lacks documentation comments for the public API functions. Adding PHPDoc style comments would improve API usability and maintainability.
Example for one of the functions:
/** * Resolves a namespace by name, creating it if it doesn't exist * * @param zend_string *name The fully qualified namespace name * @return zend_class_entry* The namespace class entry or NULL on error */ zend_class_entry *zend_resolve_namespace(zend_string *name) { // ... }
63-71: Improve error handling for interned string creation.The code doesn't check if
zend_string_init_interned()failed. While this is unlikely, adding a check would make the code more robust.Apply this diff:
zend_string *needle = zend_string_init_interned(ZSTR_VAL(current_ns.s), ZSTR_LEN(current_ns.s), 1); + if (!needle) { + zend_string_release(part); + continue; + } ns = zend_hash_find_ptr(&namespaces, needle); zend_string_release(part); if (!ns) { ns = create_namespace(needle); + if (!ns) { + zend_string_release(needle); + continue; + } ns->parent = parent_ns; zend_hash_add_ptr(&namespaces, needle, ns); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Zend/zend.c(2 hunks)Zend/zend_namespaces.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Zend/zend.c
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.c (6)
Zend/zend_execute_API.c (4)
zend_class_entry(710-722)zend_class_entry(1226-1371)zend_class_entry(1374-1377)zend_class_entry(1380-1395)Zend/zend_inheritance.c (2)
zend_class_entry(3520-3792)zend_class_entry(3887-3969)Zend/zend_compile.c (1)
zend_class_entry(1406-1446)Zend/zend_compile.h (2)
zend_initialize_class_data(983-983)name(988-988)Zend/zend_namespaces.h (3)
name(25-25)name(27-27)zend_destroy_namespaces(26-26)Zend/zend_execute.h (5)
name(37-37)name(51-51)name(52-52)name(53-53)name(422-422)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
🔇 Additional comments (1)
Zend/zend_namespaces.c (1)
41-82: Simplify namespace parsing logic.The current parsing approach is complex and difficult to follow. Consider using a more structured approach to split the namespace string by backslashes.
A cleaner approach would be:
static zend_class_entry *insert_namespace(const zend_string *name) { zend_class_entry *ns = NULL; zend_class_entry *parent_ns = global_namespace; - zend_string *part = NULL; - const char *start = ZSTR_VAL(name); - const char *end = start + ZSTR_LEN(name); - const char *pos = start; - size_t len = 0; + char *saveptr = NULL; + char *token = NULL; + zend_string *ns_copy = zend_string_dup(name, 0); smart_str current_ns = {0}; - while (pos <= end) { - if (pos == end || *pos == '\\') { - len = pos - start; - part = zend_string_init(start, len, 0); - + token = php_strtok_r(ZSTR_VAL(ns_copy), "\\", &saveptr); + while (token != NULL) { + zend_string *part = zend_string_init(token, strlen(token), 0); + if (current_ns.s) { smart_str_appendc(¤t_ns, '\\'); } smart_str_appendl(¤t_ns, ZSTR_VAL(part), ZSTR_LEN(part)); smart_str_0(¤t_ns); zend_string *needle = zend_string_init_interned(ZSTR_VAL(current_ns.s), ZSTR_LEN(current_ns.s), 1); ns = zend_hash_find_ptr(&namespaces, needle); zend_string_release(part); if (!ns) { ns = create_namespace(needle); ns->parent = parent_ns; zend_hash_add_ptr(&namespaces, needle, ns); } parent_ns = ns; - start = pos + 1; + token = php_strtok_r(NULL, "\\", &saveptr); } - pos++; } + zend_string_release(ns_copy); smart_str_free(¤t_ns); return ns; }
| static zend_class_entry *global_namespace = NULL; | ||
| static HashTable namespaces; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add thread safety guards for static variables.
Static variables could cause issues in a ZTS (Zend Thread Safety) build. Consider using TSRM macros or moving these to the executor globals structure for proper thread safety.
🏁 Script executed:
#!/bin/bash
# Check if the code is compiled with ZTS support
grep -r "TSRM" --include="*.h" Zend/
grep -r "defined(ZTS)" --include="*.h" Zend/Length of output: 2754
Thread Safety Concern in Zend/zend_namespaces.c
After verifying that the repository employs TSRM macros and ZTS-related conditionals (as seen in the grep output from various header files), it’s still notable that the following static variables remain unguarded:
static zend_class_entry *global_namespace = NULL;static HashTable namespaces;
Since these variables are defined in a file that could be used in a ZTS (Zend Thread Safety) build, they need explicit thread safety safeguards. Please update this code by either:
- Wrapping the variables with the appropriate TSRM macros (as seen in other parts of the codebase), or
- Moving these static variables into a thread-safe context (such as the executor globals structure) to ensure proper isolation across threads.
| zend_class_entry *zend_resolve_namespace(zend_string *name) { | ||
| if (global_namespace == NULL) { | ||
| global_namespace = create_namespace(zend_string_copy(zend_empty_string)); | ||
| global_namespace->lexical_scope = NULL; | ||
| zend_hash_init(&namespaces, 8, NULL, ZEND_CLASS_DTOR, 1); | ||
| zend_hash_add_ptr(&namespaces, zend_empty_string, global_namespace); | ||
| } | ||
|
|
||
| if (name == NULL || ZSTR_LEN(name) == 0) { | ||
| return global_namespace; | ||
| } | ||
|
|
||
| zend_string *lc_name = zend_string_tolower(name); | ||
| zend_class_entry *ns = zend_hash_find_ptr(&namespaces, lc_name); | ||
|
|
||
| if (!ns) { | ||
| ns = insert_namespace(lc_name); | ||
| } | ||
|
|
||
| zend_string_release(lc_name); | ||
|
|
||
| return ns; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add memory safety check after creating global namespace.
After initializing global_namespace, there's no check to ensure the memory allocation was successful. This could lead to a null pointer dereference in subsequent code.
Apply this diff to add the necessary safety check:
zend_class_entry *zend_resolve_namespace(zend_string *name) {
if (global_namespace == NULL) {
global_namespace = create_namespace(zend_string_copy(zend_empty_string));
+ if (global_namespace == NULL) {
+ return NULL;
+ }
global_namespace->lexical_scope = NULL;
zend_hash_init(&namespaces, 8, NULL, ZEND_CLASS_DTOR, 1);
zend_hash_add_ptr(&namespaces, zend_empty_string, global_namespace);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zend_class_entry *zend_resolve_namespace(zend_string *name) { | |
| if (global_namespace == NULL) { | |
| global_namespace = create_namespace(zend_string_copy(zend_empty_string)); | |
| global_namespace->lexical_scope = NULL; | |
| zend_hash_init(&namespaces, 8, NULL, ZEND_CLASS_DTOR, 1); | |
| zend_hash_add_ptr(&namespaces, zend_empty_string, global_namespace); | |
| } | |
| if (name == NULL || ZSTR_LEN(name) == 0) { | |
| return global_namespace; | |
| } | |
| zend_string *lc_name = zend_string_tolower(name); | |
| zend_class_entry *ns = zend_hash_find_ptr(&namespaces, lc_name); | |
| if (!ns) { | |
| ns = insert_namespace(lc_name); | |
| } | |
| zend_string_release(lc_name); | |
| return ns; | |
| } | |
| zend_class_entry *zend_resolve_namespace(zend_string *name) { | |
| if (global_namespace == NULL) { | |
| global_namespace = create_namespace(zend_string_copy(zend_empty_string)); | |
| if (global_namespace == NULL) { | |
| return NULL; | |
| } | |
| global_namespace->lexical_scope = NULL; | |
| zend_hash_init(&namespaces, 8, NULL, ZEND_CLASS_DTOR, 1); | |
| zend_hash_add_ptr(&namespaces, zend_empty_string, global_namespace); | |
| } | |
| if (name == NULL || ZSTR_LEN(name) == 0) { | |
| return global_namespace; | |
| } | |
| zend_string *lc_name = zend_string_tolower(name); | |
| zend_class_entry *ns = zend_hash_find_ptr(&namespaces, lc_name); | |
| if (!ns) { | |
| ns = insert_namespace(lc_name); | |
| } | |
| zend_string_release(lc_name); | |
| return ns; | |
| } |
| void zend_destroy_namespaces(void) { | ||
| if (global_namespace == NULL) { | ||
| return; | ||
| } | ||
|
|
||
| zend_hash_destroy(&namespaces); | ||
| global_namespace = NULL; | ||
| } |
There was a problem hiding this comment.
Consider adding a more complete cleanup in zend_destroy_namespaces.
The function doesn't fully clean up the global_namespace if it was allocated with malloc. This could lead to memory leaks.
Apply this diff:
void zend_destroy_namespaces(void) {
if (global_namespace == NULL) {
return;
}
zend_hash_destroy(&namespaces);
+ /* If using emalloc, this would be efree(global_namespace) */
+ free(global_namespace);
global_namespace = NULL;
}Note: This reinforces why using emalloc instead of malloc is important - with Zend memory management, the cleanup would be handled automatically through ZEND_CLASS_DTOR.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void zend_destroy_namespaces(void) { | |
| if (global_namespace == NULL) { | |
| return; | |
| } | |
| zend_hash_destroy(&namespaces); | |
| global_namespace = NULL; | |
| } | |
| void zend_destroy_namespaces(void) { | |
| if (global_namespace == NULL) { | |
| return; | |
| } | |
| zend_hash_destroy(&namespaces); | |
| /* If using emalloc, this would be efree(global_namespace) */ | |
| free(global_namespace); | |
| global_namespace = NULL; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/classes/.idea/deployment.xml (1)
5-11: Element Naming Consistency IssueWithin the
<serverData>block, there's a child element<serverdata>(lines 6 and 10) whose tag casing does not match that of its parent. XML is case-sensitive, so if uniformity is desired, consider changing<serverdata>to<serverData>. If this is intentional, adding a brief comment or documentation note would help future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Zend/zend_namespaces.c(1 hunks)tests/classes/.idea/deployment.xml(1 hunks)tests/classes/.idea/php.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/classes/.idea/php.xml
- Zend/zend_namespaces.c
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (3)
tests/classes/.idea/deployment.xml (3)
1-2: XML Declaration & Project VersionThe XML declaration and opening
<project>element with version "4" are correctly formatted. Please verify that the version number aligns with your IDE’s expected configuration schema.
3-4: Component & Server Data DeclarationThe
<component name="PublishConfigData">and its immediate child<serverData>element are correctly set up. Ensure these align with your deployment requirements.
12-14: Closing Tags & Overall StructureThe closing tags for
<serverData>,<component>, and<project>are proper and maintain the intended hierarchy. The overall structure meets IDE configuration expectations.
```
ext/gd/libgd/gd.c:2275:14: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
#0 0x5d6a2103e1db in php_gd_gdImageCopy /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd.c:2275
#1 0x5d6a210a2b63 in gdImageCrop /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd_crop.c:57
#2 0x5d6a21018ca4 in zif_imagecrop /home/dcarlier/Contribs/php-src/ext/gd/gd.c:3575
#3 0x5d6a21e46e7a in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1337
#4 0x5d6a221188da in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:57246
#5 0x5d6a221366bd in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:61634
#6 0x5d6a21d107a6 in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1895
#7 0x5d6a21a63409 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2529
#8 0x5d6a22516d5e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:966
php#9 0x5d6a2251981d in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1341
php#10 0x7f10d002a3b7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
php#11 0x7f10d002a47a in __libc_start_main_impl ../csu/libc-start.c:360
php#12 0x5d6a20a06da4 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2806da4) (BuildId: d9a79c7e0e4872311439d7313cb3a81fe04190a2)
```
close phpGH-18006
Todos:
Outer\MiddleandOther\Middlethe same when usingMiddleon inherited classes)Summary by CodeRabbit
New Features
Bug Fixes
Tests