[PHP] FIXES #9171 Return type not added to constructor override#9234
Closed
DamImpr wants to merge 74 commits intoapache:masterfrom
Closed
[PHP] FIXES #9171 Return type not added to constructor override#9234DamImpr wants to merge 74 commits intoapache:masterfrom
DamImpr wants to merge 74 commits intoapache:masterfrom
Conversation
|
Good job! very useful |
mbien
added
PHP
Contributor
|
@DamImpr thanks for taking are and working on this. The CI/CD run showed errors. The execution fails in the From reading the code the problem is, that The tests can be run from the IDE by opening the context menu of the project and choose Test there. Be warned: The PHP test suite is extensive. The runs on github take in the range of 45-60 minutes. You can run individual test files be invoking "Test file" on the corresponding java file or the context menu when the file is open. You can also run individual tests be using "Run focused test method from the context menu when you click inside the method body. It would be good if you could add a testcase for the expected behavior.
|
…lso the class name
Author
|
@matthiasblaesing thank you for your advice. I have adapted the fix so that it is compliant with the current PHP test suite. A commit will follow with a test case following your instructions. |
Contributor
|
Did you test your change? For me it did not. I traced the code that is invoked and I get this:
|
…h the "Override & implement method" feature.
Author
|
@matthiasblaesing The generated code now correctly omits the return type in the constructor. I have not yet been able to find where the list of methods is generated, but it is still displayed |
Author
|
I still have one test that fails, I need to work on it some more. |
Contributor
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
Thank you for the update. The change works, I'd like to see a test, but as this needs the PHP stubs I'm not sure it is feasible.
In any case, please squash the changes and rebase them onto current master. Please ensure that there is not only the description what was changes, but also why it was changed and is now correct.
Please then also check the description of the PR if it is still correct and if not adjust it.
dev builds were zipped twice due to a limitation of the action, this fixes it with the update. (new "archive: false" param)
Removes duplicated dependency declarations in - ide/languages.toml - ide/xml.lexer - java/api.maven - java/gradle.java - webcommon/javascript2.vue
- Additional package lucene-analysis-common required (KeywordAnalyzer, WhitespaceAnalyzer, PerFieldAnalyzerWrapper, CharTokenizer, LimitTokenCountAnalyzer) - FieldTypes were introduced to carry the field behavior (tokenization state, indexing options, storage settings) - BooleanQuery construction moved to builder - Collector interface was modified - TermEnum was replaced by TermsEnum - Terms are stored per field, so queries have to be specified with the target field - QuerySelectors were replaced by String sets - RAMDirecory was removed and is replaced by ByteBufferDirectories - Lock handling was reworked - Remove StoppableConvertor from Lucene Support - Work around potentially excessivily large terms in index documents. (observed was a single `usage` entry for a JS document with about 40_000 characters) Co-authored-by: Laszlo Kishalmi <laszlo.kishalmi@gmail.com> Co-authored-by: Michael Bien <mbien42@gmail.com>
…ing comments for the preceding member are ignored.
… scope' due to enum switch target typing.
The test depends on the downloadable API documentation from node js and the golden files are only correct for a limited version range.
- toml had no new-file template - set requireProject = false for both toml and yaml - moved registration from layer to annotation - fixed trivial layer ordering conflict (jflex grammar)
Added --module-path and --add-modules in order to start Glassfish after the bootstrap was redesigned.
skipped - xml.tax - xml.text.obsolete90 - o.n.agent - dlight.nativeexecution consolidated compiler args property edited db.mysql sig file (Thread.destroy() and stop() removal) dropped libwrappers (without sources) to their bytecode level if they had a javac property defined.
- about 100ms faster startup (main method duration measured) and technically more correct anyway - use JFrame to get double buffering (no progress bar flicker during repaint) and we can use the opportunity to initialize swing asynchronously while NB is busy bootstrapping. - other change: splash window is no longer modal - code renovations most changes come from the fact that the model needed to be extracted from UI code so that it can be updated from any thread while the UI paints on EDT.
The warning said "No enabled eligible for injection beans are found", because the filter class did not check "jakarta." namespace
the tests can run on regular JDKs, GraalVM's polyglot features became dependencies in pasts. the modules are already covered by the java, debugger, ide, profiler and platform jobs (only the nashorn module needed to be moved).
Move the setting of awtAppClassName for XToolkit into Main so that it is set before splash and import dialogs are created. This fixes issues with duplicate dock icons caused by incorrect WM_CLASS derived from this field value.
Export-Package entries are not just comma separated values, but can contain parameters, which themselves can contain commas in quoted strings. These cases need to be handled correctly.
- use Data*Streams instead of Object*Streams - simplify file format (no XML) - code renovation and other minor optimizations results in 4x faster cache loading times (readCache() and calculateParents() methods) minor: - ModuleManager.EnableContext: List -> Set for field solely used for contains() in inner loop - Module: data loading can synchronize on instance instead of class (DATA_LOCK no longer static) note: The IO utility class calls Dependency#create using MethodHandle to avoid having to make it public API or introduce a split package.
…the CRaCCheckpointTo Option
## Skip firing events for up-to-date Skip firing events for up-to-date files that are not yet in the cache, since UPTODATE is the default for managed files. This drastically reduces the time spent in the refreshStatusesBatch method on big repositories executed when Commit dialog opens. For example, on the Netbeans repository, from around 8 seconds to 20ms. ## Batch status change notifications Batch status change notifications to avoid per-file event overhead. Replace per-file PROP_FILE_STATUS_CHANGED firing in refreshStatusesBatch with a single PROP_FILES_STATUS_CHANGED batch event, eliminating 100k redundant propertyChange/schedule/SwingUtilities.invokeLater calls on first load. This improves performance a bit because it eliminates many method calls. However, in the end, the number of files changed is the same so the event handler still needs to process all of them. ## Move status updates to a background thread File status update requires I/O operation to refresh files metadata from FS. This is very slow when many files need to be updated. Moving them to a background thread offloads this slow operation from the main thread. Updates can run asynchronously without blocking the main thread that fires the status events, they just update UI hints, they have no impact no behavior. For the whole Netbeans repository, this shortens the time it takes to complete the firePropertyChange event from 6 seconds to 1 second.
Speeds up GitClient.getStatus() by deferring expensive evaluation of object Ids, which often compute file content hash, to evaluate them lazily only when needed. On Netbeans repository with a lot of files, this speeds up GitClient.getStatus() execution from 4 seconds to 1 second.
Newer bouncy castle JARs declare an import of the java.io package. This is rejected by the manifest verification in the equinox version NetBeans uses wrapped into Netbinox. This change deactivates the check in equinox that rejects that entry. Additionally the instructions for building the patched equinox version were updated. Closes: apache#8894
adjusted RevWalk max flags constant since it diverges from javadoc and added tripwire test in case it changes again in future.
TreeView will now wait a little bit before showing the wait cursor. Co-authored-by: Laszlo Kishalmi <laszlo.kishalmi@gmail.com>
- use SortedSet#removeFirst instead of getFirst()+remove() - remove ThreadDeath and AccessController.doPrivileged() usage - convert Maps of type WeakHashMap to Set where applicable - generics rawtype warning fixes, diamonds, overrides and similar minor renovations - javadoc typos
Update FlatLaf properties to set the colour of the drag target indicator to derive from the active tab underline colour (usually accent colour). Also make the stroke weight configurable (but kept default in FlatLaf).
- sets -XX:+UseCompactObjectHeaders which is supported from JDK 25 and later - NB sets XX:+IgnoreUnrecognizedVMOptions already which means it will be silently ignored on older JDKs - (experimental on EOL JDK 24, but it won't enable there without also setting the flag to enable experimental options - which we don't do) tested it on NB since JDK 24 without issues
add java 26 to the wizard and set gradle 9.4.0 as JDK 26 compatible
* added Faces 4 ClientWindowScoped (with warning if project uses JSF 3.0 or older) * disabled "Add data to configuration file" check for Faces 4+ projects * moved some logic from switch/maps to enum fields * small refactor/modernization of the code Add NetBeansProjects dir to Favorites tab - projects dir is now registered as favorite - fixed bug: when a custom project dir location is specified by setting 'netbeans.projects.dir', the dir is now automatically created, instead of falling back to the default dir, which gave the impression that the property isn't working Payara Micro Maven Plugin v2.5.3 support Increment spec versions for Apache NetBeans 31 developement Update issue form for NB 30 release candidates
DamImpr
force-pushed
the
fix-php-override-constructor
branch
from
a5f16bc to
fd2b690
Compare
Author
|
@matthiasblaesing I’d rather create a new, clean pull request |
Contributor
|
@DamImpr sure, thank you and take your time. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
17 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As reported in issue #9171 adding the return type to the constructor in a PHP project caused a conflict with the ‘override methods’ feature, making the code incompatible with PHP inheritance rules.
This PR removes the return type from the constructor, restoring compatibility with PHP conventions and resolving the reported bug.
Effect: the constructor can now be correctly overridden in child classes, as expected by the language.