- Issue created by @bceyssens
- ๐ง๐ชBelgium bceyssens Genk ๐ง๐ช
Using the AllowDynamicProperties attributes on the Result class fixes the issue for now.
- First commit to issue fork.
- Merge request !129Issue #3336646: Deprecated function: Creation of dynamic property โ (Open) created by ankitv18
- Assigned to ankitv18
- Status changed to Needs work
almost 2 years ago 8:58am 2 March 2023 - Issue was unassigned.
- Status changed to Needs review
almost 2 years ago 9:45am 2 March 2023 - Status changed to RTBC
over 1 year ago 7:14am 9 May 2023 - last update
over 1 year ago 424 pass, 2 fail - last update
over 1 year ago 424 pass, 2 fail - Status changed to Closed: duplicate
over 1 year ago 6:11pm 17 August 2023 - heddn Nicaragua
This is a duplicate of ๐ PHP 8.2 compatibility Fixed . Which is further along in its work.
- ๐จ๐ฆCanada Liam Morland Ontario, CA ๐จ๐ฆ
Perhaps this simple solution should be committed to the 2.0.x branch to get that release fully supporting PHP 8.2.
- Status changed to Needs review
about 1 year ago 4:58pm 4 November 2023 - ๐ง๐ฌBulgaria pfrenssen Sofia
๐ PHP 8.2 compatibility Fixed has been committed to 3.0.x. Let's reopen this to make 2.0.x also PHP 8.2 compatible.
- ๐จ๐ญSwitzerland colorfield Lausanne
Manually tested on a project with https://git.drupalcode.org/project/facets/-/merge_requests/129
RTBC+1
- Status changed to RTBC
about 1 year ago 4:33pm 23 November 2023 - ๐ฌ๐งUnited Kingdom SirClickALot Somerset
I am using
2.06
of Facets and I too have just updated my PHP to8.22
and I see the same error report which makes my site unusable :-) ...Deprecated function: Creation of dynamic property Drupal\facets\Result\Result::$transliterateDisplayValue is deprecated in Drupal\facets\Plugin\facets\processor\DisplayValueWidgetOrderProcessor->sortResults() (line 72 of modules\contrib\facets\src\Plugin\facets\processor\DisplayValueWidgetOrderProcessor.php)....
I have applied (or at least attempted to) the patch from #10 but it did not make any difference - according to my local GIT, it made no changes to the...
"<site>/modules/contrib\facets/src\Result/Result.php"
file?Can anyone please advise here or do I just need to wait for the next release of the 2.x version?
Thanks
- ๐ฉ๐ฐDenmark ressa Copenhagen
Another +1 for RTBC, the MR 129 fixes the error, and would be great to get in the module, to avoid flooding the logs with warnings.
@SirClickalot: I downloaded and applied the patch like this:
wget https://git.drupalcode.org/project/facets/-/merge_requests/129.diff patch -p1 < 129.diff
- ๐ฆ๐นAustria agoradesign
This should be the only way to apply patches :) https://medium.com/appseed-io/apply-drupal-9-patches-with-composer-7b1e5...
- ๐ฉ๐ฐDenmark ressa Copenhagen
Of course, always use Composer :) I added the example just to verify if it applies or not, and add a note about that. The steps are documented in Using Composer to Install Drupal and Manage Dependencies > Patching Drupal core and modules โ
This patch fixes the error for me
https://www.drupal.org/files/issues/2023-08-16/3336646-function-is-depre... โ
Thanks!- ๐บ๐ธUnited States millerrs
Another RTBC +1. It would be great to get this in the 2.x branch.
- ๐ซ๐ทFrance MacSim
Applying the patch https://git.drupalcode.org/project/facets/-/merge_requests/129.diff fixed the issue on my side as well
RTBC +1
- ๐บ๐ธUnited States jimafisk
The patch https://git.drupalcode.org/project/facets/-/merge_requests/129.diff worked for me! Thanks!
- ๐ซ๐ทFrance fmb Perpinyร , Catalonia, EU
While this ad hoc patch will most certainly fix plugins provided by the Facets module itself, it is not extensible, in the sense that custom sort plugins will be unable to dynamically set properties on Result objects, so they will have to deal with this in a different way. It should be noted that at the time you feel the urge to write such a custom plugin, you generally follow the example of a plugin provided by the main module.
I do not know, maybe we could have a general purpose property, e.g. an array, or a public method to deal with a "bag" of attributes on a Result object? That way all plugins, whether they ship with the Facets module or another one, could follow exactly the same method.
- ๐บ๐ธUnited States DamienMcKenna NH, USA
Patch #2 is the wrong approach, #12 and the merge request are the correct approach.
- ๐บ๐ธUnited States DamienMcKenna NH, USA
@FMB: Classes that extend one class can add new properties to it, that's how OOP in PHP works. So if someone extends the Facets plugins and uses dynamic properties, all they have to do is define the new properties in their new class, it's literally that easy.
- ๐ซ๐ทFrance fmb Perpinyร , Catalonia, EU
@DamienMcKenna: thanks for your answer. Sure, I am quite aware of that ^^ But when you write a sort plugin, you generally extend SortProcessorPluginBase, and most importantly implement SortProcessorInterface, which enforces a sortResults method, which takes two Drupal\facets\Result\Result arguments. The bottom line is you have no choice but to use a Result object (unless you inherit from this interface and so on...), and even if you could extend that one, it would not be particularly handy to deal with your own Result class when all you wanted was precisely to reuse the machinery provided by the module to implement a little business logic.
- ๐บ๐ธUnited States DamienMcKenna NH, USA
That's still unrelated to this issue and belongs in a support request.
- ๐ซ๐ทFrance fmb Perpinyร , Catalonia, EU
I beg to differ. This is totally related in the sense that we should look for a way to adapt sort plugins to PHP 8.2, whether they ship with the Facets module or not. The proposed solution is ad hoc in the sense that it statically adds properties which are not directly related to what the object does, only providing a feeling of superficial "compliance" with what is now dictated by PHP.
I dealt with my own plugins the best I could, but IMHO it does not make sense that the same mechanism cannot be used as in the main module. Plus it is a source of errors for people who will take Facets plugins as an example in the future.
- ๐บ๐ธUnited States DamienMcKenna NH, USA
These are just deprecations to help you be aware of forthcoming changes to how PHP itself works. When PHP 9 is released and dynamic attributes are no longer technically possible, will it be the fault of this module's developers that someone else extended the classes and did so improperly, ignoring the deprecation messages? Using the AllowDynamicProperties property ignores the reality that the PHP language is changing and code needs to be updated.
Individual developers are responsible for the code they write, not anyone else.
The solution provided here is appropriate and RTBC.
- ๐ซ๐ทFrance fmb Perpinyร , Catalonia, EU
I have already dealt with the code which is under my responsibility. Now, if you read my proposal in #25, you will see that allowing dynamic properties is not what I suggest.
- ๐บ๐ธUnited States DamienMcKenna NH, USA
Your suggestions are out of scope for this issue which is focused on making the module run on PHP 8.2 without errors or notices, feel free to bring up other topics in other issues.
- ๐ฆ๐นAustria agoradesign
adding a PS to #33 - it is both totally unrealistic and imho semantically totally wrong refactoring the architecture in branch 2.x to allow kind of dynamic properties in a future-safe way. So please FMB, feel free to bring in your ideas in a separate issue either for version 3.x or maybe a future 4.x branch, and anyone here will really appreciate your efforts, but as Damien already stated, this issue is about making the module PHP 8.2 compatible, so that we are not forced to switch to 3.x immediately, which caused me some problems a while ago. And the proposed patch does exactly that. It adds some properties, and doing this does not make anything worse regarding class inheritance.
imho the only thing that is discussable in that matter, that we should think about not only adding the two class properties as proposed, but also add the AllowDynamicProperties PHP attribute, so that any custom extensions are safe regarding this matter in PHP 8.2
Either way we decide, this is RTBC and will hopefully land soon :)
- ๐ซ๐ทFrance fmb Perpinyร , Catalonia, EU
I was not aware of this, but now you mention it @agoradesign, such a mechanism as I was trying to describe already exists on 3.x, thanks for pointing this out, it does make perfect sense now.
- ๐ซ๐ทFrance fmb Perpinyร , Catalonia, EU
In totally unrelated news, I suggest to use this link for the patch https://git.drupalcode.org/project/facets/-/merge_requests/129.diff?diff... (the diff_id freezes it in a given state over time).
- ๐ฆ๐นAustria agoradesign
oh yes, you're right. didn't know that too... ok, then everything's fine now :)
- ๐บ๐ธUnited States mortona2k Seattle
@FMB - Can you please enlighten us on how to find the diff_id? That looks super useful, but I'm digging through the page source and gitlab docs and have no idea how to find it.
- ๐ซ๐ทFrance MacSim
@mortona2k 129 is the id of the merge request ;)
Basically you just need to clic on the merge request and add ".diff" to the url - ๐ฉ๐ฐDenmark ressa Copenhagen
Thanks @MacSim, but it's not clear where
diff_id=648306
can be found ... - ๐ซ๐ทFrance MacSim
@ressa Sorry I didn't realised that second id.
I don't know where he found that but you don't need it
The diff is available here https://git.drupalcode.org/project/facets/-/merge_requests/129.diff and it's the same than https://git.drupalcode.org/project/facets/-/merge_requests/129.diff?diff... ;) - ๐ฉ๐ฐDenmark ressa Copenhagen
Yes, the patches look the same, I am not sure it's possible to pin a
.diff
file to a specific commit point in history, yet. The problem is that the patch MR content can change on each commit, and that's a security risk:Warning:
Hotlinking to remotely hosted patches, whether on Drupal.org or another service is not recommended. A remotely hosted patch file might become inaccessible due to an outage, or even deletion, and in a worse case scenario the contents of a remotely hosted patch file can be changed which creates a risk of malicious code.For the integrity of your build chain, it is best practice to commit your patch files to a local repository.
from https://www.drupal.org/docs/develop/using-composer/manage-dependencies#p... โ
Also, I have now read the article Apply Drupal 9 patches with composer closer, shared in #19 ๐ PHP 8.2 compatibility for the 2.0.x branch Fixed , and that is NOT how to do it. Apply it with Composer, yes -- but the only safe method is to download the patch, not hotlink it to a remote hosted patch.
- ๐ซ๐ทFrance fmb Perpinyร , Catalonia, EU
Can you please enlighten us on how to find the diff_id? That looks super useful, but I'm digging through the page source and gitlab docs and have no idea how to find it.
@mortona2k sure, go to the changes tab for this MR. Under the tabs and before the code, it says "Compare 2.0.x and latest version". Click on "latest version", and then again on "latest version" in the dropdown menu that appears. You can now see the diff_id in the URL: https://git.drupalcode.org/project/facets/-/merge_requests/129/diffs?dif.... At this point, just replace "/diffs" by ".diff" and there you are.
True, you may commit patches in your own repository, but specifying a diff_id makes it difficult to turn the patch into malicious code later on.
- ๐บ๐ธUnited States mortona2k Seattle
@FMB Thank you very much.
I generally use a MR diff as a patch and downloaded it to /patches. But my patches were named after the MR ID, and I had no way to tell if there was an update or keep track of previously working versions.
People often upload patch files to a thread with the comment name. I think using the MR + diff_id would be much more useful for identifying where it came from.
If Drupal.org/Gitlab could add the diff_id to the patch link in these issues, that would help people linking directly to the diff avoid getting bad commits added to the branch.
- ๐ฉ๐ฐDenmark ressa Copenhagen
@FMB: Following your steps, I get the same patch, which is the latest:
Compare 2.x and latest
- https://git.drupalcode.org/project/facets/-/merge_requests/129/diffs?dif...
- https://git.drupalcode.org/project/facets/-/merge_requests/129.diff?diff...
Compare 2.x and version 1
- https://git.drupalcode.org/project/facets/-/merge_requests/129/diffs?dif...
- https://git.drupalcode.org/project/facets/-/merge_requests/129.diff?diff...
Both
129.diff?diff_id=648306
and129.diff?diff_id=419569
result in this, where version 1 (diff_id=419569
) should beprotected $transliterateDisplayValue;
:diff --git a/src/Result/Result.php b/src/Result/Result.php index 334dc68559ba310b8743a188a66a2f1075e059b1..6ae88d99d980745766da700f8e2eb5eef779ebfa 100644 --- a/src/Result/Result.php +++ b/src/Result/Result.php @@ -73,6 +73,20 @@ class Result implements ResultInterface { */ protected $children = []; + /** + * The facet transliterate display value. + * + * @var string + */ + public $transliterateDisplayValue; + + /** + * The term weight. + * + * @var int + */ + public $termWeight; + /** * Constructs a new result value object. *
- ๐ซ๐ทFrance fmb Perpinyร , Catalonia, EU
I tried with another MR with more commits. Turns out you are right, this works for the /diffs page but apparently not for the page (.diff), my bad. Thanks for investigating this.
- ๐ฉ๐ฐDenmark ressa Copenhagen
Thanks for confirming, let's hope Add support for diff_id in links to file in merge requests will add this before too long:
If you switch between different versions of a merge request you won't be able to link to a file or a file line for this specific version. The link to the file or the file line always points to the latest version. We should incorporate
diff_id
parameter for these links to support linking to different versions of the MR in files. - ๐ฉ๐ชGermany osopolar ๐ฉ๐ช GER ๐
Copy of patch from https://git.drupalcode.org/project/facets/-/merge_requests/129.diff (#12) attached, to be used with composer, see "Patches from drupal.org merge request URLs are dangerous?".
- ๐ณ๐ดNorway steinmb
RTBC, should be a easy merge. Can we get this in? LGTM
- Status changed to Fixed
10 months ago 12:14pm 8 March 2024 -
borisson_ โ
committed c5eb3a4c on 2.0.x
Issue #3336646 by ankitv18, bceyssens, FMB, ressa, DamienMcKenna: PHP 8....
-
borisson_ โ
committed c5eb3a4c on 2.0.x
Automatically closed - issue fixed for 2 weeks with no activity.